2012/11/28 Jeff Layton <jlayton@xxxxxxxxxx>:
> On Wed, 28 Nov 2012 16:12:56 +0400
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> 2012/11/28 Jeff Layton <jlayton@xxxxxxxxxx>:
>> > On Tue, 27 Nov 2012 18:57:22 +0400
>> > Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>> >
>> >> 2012/11/27 Jeff Layton <jlayton@xxxxxxxxxx>:
>> >> > On Tue, 27 Nov 2012 10:50:28 +0400
>> >> > Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>> >> >
>> >> >> If we have a read oplock and set a read lock in it, we can't write to the
>> >> >> locked area - so, filemap_fdatawrite may fail with a no information for a
>> >> >> userspace application even if we request a write to non-locked area. Fix
>> >> >> this by replacing it with filemap_write_and_wait call and sending non-page
>> >> >> write in a error case.
>> >> >>
>> >> >> While this may end up with two write requests to the server, we can be sure
>> >> >> that our data will be the same at the server and the page cache - the next read
>> >> >> on this file gets the valid data.
>> >> >>
>> >> >> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>> >> >> ---
>> >> >> fs/cifs/file.c | 10 ++++------
>> >> >> 1 file changed, 4 insertions(+), 6 deletions(-)
>> >> >>
>> >> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> >> >> index f8fe1bd..89efd85 100644
>> >> >> --- a/fs/cifs/file.c
>> >> >> +++ b/fs/cifs/file.c
>> >> >> @@ -2511,14 +2511,12 @@ cifs_strict_writev(struct kiocb *iocb, const struct iovec *iov,
>> >> >> */
>> >> >> if (!cinode->clientCanCacheAll && cinode->clientCanCacheRead) {
>> >> >> ssize_t written;
>> >> >> - int rc;
>> >> >>
>> >> >> written = generic_file_aio_write(iocb, iov, nr_segs, pos);
>> >> >> - rc = filemap_fdatawrite(inode->i_mapping);
>> >> >> - if (rc)
>> >> >> - return (ssize_t)rc;
>> >> >> -
>> >> >> - return written;
>> >> >> + /* try page write at first */
>> >> >> + if (!filemap_write_and_wait(inode->i_mapping))
>> >> >> + return written;
>> >> >> + /* page write failed - try from pos to pos+len-1 */
>> >> >> }
>> >> >> #endif
>> >> >>
>> >> >
>> >> > Bleh -- nasty. I guess this will work though...
>> >> >
>> >> > Wonder if there's some way to populate the cache and then just mark the
>> >> > pages clean without sending out writes? That would be a better solution
>> >> > IMO, but I guess we can live with this for now...
>> >> >
>> >> > Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
>> >>
>> >> Thanks for reviewing the patchset.
>> >>
>> >> As for this patch I have the followon patch:
>> >>
>> >> http://git.altlinux.org/people/piastry/public/?p=cifs-2.6.git;a=commitdiff;h=11673376e6840c686f0c6e51892b16ad1945887e
>> >>
>> >> that allows us to populate the page cache without making pages dirty.
>> >> I didn't have enough time to test it well (that's why I haven't posted
>> >> it yet) - going to do it tomorrow. But you are welcome to comment the
>> >> approach.
>> >>
>> >
>> > The only thing that makes me wary here is that you're setting this flag
>> > on the inode. Could there ever be a situation where another task might
>> > be writing to this inode at the same time and needs to set them dirty?
>> >
>> > If not, then I'm not sure I see the need for a new bool in the inode.
>> > It might be simpler to just check what sort of oplock you have in
>> > write_end instead.
>> >
>> > There's also a lot of logic around what sort of locking you're doing
>> > here too. I think we ought to do the same sort of I/O regardless of
>> > whether POSIX locks are being used or not.
>> >
>>
>> There are some places where VFS code uses write_end call (through
>> pagecache_write_end) but I didn't find any place where cifs code can
>> hit it. So, I think we can assume now that cifs_write_end is called
>> only from generic_file_aio_write codepath. If so, we can be sure that
>> only on process may want to set pages dirty through cifs_write_end due
>> to i_mutex lock.
>>
>> It seems your are right and we can use clientCanCacheAll value
>> directly from cifs_write_end - will make changes. Also, I think I can
>> merge these two patches into one.
>>
>> Thanks.
>>
>
> Note though, that it matters what sort of cache= option is in force
> too. If cache=loose then you *do* want to set the page dirty.
This makes sense, thanks.
--
Best regards,
Pavel Shilovsky.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
