On Thu, 2013-08-08 at 14:11 -0400, Jeff Layton wrote:
> On Wed, 7 Aug 2013 17:14:24 -0400
> Trond Myklebust <Trond.Myklebust@xxxxxxxxxx> wrote:
>
> > If a cache invalidation is triggered, and we happen to have a lot of
> > writebacks cached at the time, then the call to invalidate_inode_pages2()
> > will end up calling ->launder_page() on each and every dirty page in order
> > to sync its contents to disk, thus defeating write coalescing.
> > The following patch ensures that we try to sync the inode to disk before
> > calling invalidate_inode_pages2() so that we do the writeback as efficiently
> > as possible.
> >
> > Reported-by: William Dauchy <william@xxxxxxxxx>
> > Reported-by: Pascal Bouchareine <pascal@xxxxxxxxx>
> > Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> > Tested-by: William Dauchy <william@xxxxxxxxx>
> > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> > v2: Add check for regular file as per Jeff Layton's suggestion.
> > v3: Minor cleanup and add Jeff as a reviewer
> >
> > fs/nfs/inode.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > index af6e806..3ea4f64 100644
> > --- a/fs/nfs/inode.c
> > +++ b/fs/nfs/inode.c
> > @@ -963,9 +963,15 @@ EXPORT_SYMBOL_GPL(nfs_revalidate_inode);
> > static int nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping)
> > {
> > struct nfs_inode *nfsi = NFS_I(inode);
> > -
> > + int ret;
> > +
> > if (mapping->nrpages != 0) {
> > - int ret = invalidate_inode_pages2(mapping);
> > + if (S_ISREG(inode->i_mode)) {
> > + ret = nfs_sync_mapping(mapping);
> > + if (ret < 0)
> > + return ret;
> > + }
> > + ret = invalidate_inode_pages2(mapping);
> > if (ret < 0)
> > return ret;
> > }
>
> It occurs to me that we have several places that call nfs_sync_mapping
> without checking S_ISREG. Are they potentially problematic?
>
> Might it make more sense to move the S_ISREG test inside of
> nfs_sync_mapping and just have it "return 0" when it's not a regular
> file?
I see 5 callers of nfs_sync_mapping() aside from the above: 2 are in the
O_DIRECT code, the other 3 are all in the file locking code. AFAICS,
none of those can ever be fed to non-regular files.
Am I missing anything?
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
