It don't think it is a correctness issue - if close wants to do an
fsync why do we have a flush routine at all? close is the only place
flush is called. This seems very wrong to require additional
semantics beyond Unix semantics here (and slows close performance way
down unnecessarily). Even if we go async we would initiate i/o on
these before we return close to the user - and we are not going to
close the network handle of course until all network writes complete.
At a minimum, we don't need to do an fsync (flush with wait) on close
if there is more than one handle to that inode open - and should be
able to just do flush
On Fri, Sep 24, 2010 at 1:11 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Fri, 24 Sep 2010 12:58:55 -0500
> Steve French <smfrench@xxxxxxxxx> wrote:
>
>> We need to see the performance impact. As you say cifs_writepages is
>> synchronous so we should be ok without it. Any test results
>> before/after?
>>
>
> No, I haven't tested this for performance. It is a correctness issue
> though. We absolutely can't put the last reference to the last open
> filehandle without flushing all of the data first.
>
> My expectation here though is that this may help performance in some
> cases since this patch also has it skip the flush on files open
> read-only.
>
>> On Fri, Sep 24, 2010 at 7:59 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>> > It's possible that the kernel will call cifs_close on a filp while there
>> > is still dirty data attached to the inode. If it's the last writeable
>> > filp for an inode, then we may have no way to flush this data to the
>> > server.
>> >
>> > Thus, simply doing a filemap_fdatawrite in the .flush operation isn't
>> > sufficient. The client needs to wait for it to complete before
>> > returning. OTOH, there's no real need to perform a flush on a
>> > filehandle that's read-only.
>> >
>> > What currently saves us on this is the fact that cifs_writepages is
>> > basically synchronous. If and when we ever switch to an async writeback
>> > model, this will need to be fixed, so we might as well go ahead and
>> > fix it now.
>> >
>> > When flushing a writable filp, wait for the writeback to complete before
>> > returning to ensure that the filehandle stays around until the writeback
>> > is complete.
>> >
>> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>> > ---
>> > fs/cifs/file.c | 17 +++++++----------
>> > 1 files changed, 7 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> > index 80856f1..8084a8d 100644
>> > --- a/fs/cifs/file.c
>> > +++ b/fs/cifs/file.c
>> > @@ -1737,16 +1737,13 @@ int cifs_flush(struct file *file, fl_owner_t id)
>> > struct inode *inode = file->f_path.dentry->d_inode;
>> > int rc = 0;
>> >
>> > - /* Rather than do the steps manually:
>> > - lock the inode for writing
>> > - loop through pages looking for write behind data (dirty pages)
>> > - coalesce into contiguous 16K (or smaller) chunks to write to server
>> > - send to server (prefer in parallel)
>> > - deal with writebehind errors
>> > - unlock inode for writing
>> > - filemapfdatawrite appears easier for the time being */
>> > -
>> > - rc = filemap_fdatawrite(inode->i_mapping);
>> > + /*
>> > + * if the file is writable, then flush any dirty data to ensure that
>> > + * the filehandle isn't closed out before writeback completes.
>> > + */
>> > + if (file->f_mode & FMODE_WRITE)
>> > + rc = filemap_write_and_wait(inode->i_mapping);
>> > +
>> > /* reset wb rc if we were able to write out dirty pages */
>> > if (!rc) {
>> > rc = CIFS_I(inode)->write_behind_rc;
>> > --
>> > 1.7.2.3
>> >
>> >
>>
>>
>>
>
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>
--
Thanks,
Steve
--
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
