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.
--
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
