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.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
