Re: [PATCH 6/6] NFS: return -EAGAIN when skipped commit innfs_commit_unstable_pages()

[Wu Fengguang]

Hi Trond,

On Tue, Apr 19, 2011 at 11:29:07AM +0800, Trond Myklebust wrote:
> On Tue, 2011-04-19 at 11:00 +0800, Wu Fengguang wrote:
> > plain text document attachment (nfs-fix-write_inode-retval.patch)
> > It's probably not sane to return success while redirtying the inode at
> > the same time in ->write_inode().
> > 
> > CC: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> > Signed-off-by: Wu Fengguang <fengguang.wu@xxxxxxxxx>
> > ---
> >  fs/nfs/write.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > --- linux-next.orig/fs/nfs/write.c	2011-04-19 10:18:16.000000000 +0800
> > +++ linux-next/fs/nfs/write.c	2011-04-19 10:18:32.000000000 +0800
> > @@ -1519,7 +1519,7 @@ static int nfs_commit_unstable_pages(str
> >  {
> >  	struct nfs_inode *nfsi = NFS_I(inode);
> >  	int flags = FLUSH_SYNC;
> > -	int ret = 0;
> > +	int ret = -EAGAIN;
> >  
> >  	if (wbc->sync_mode == WB_SYNC_NONE) {
> >  		/* Don't commit yet if this is a non-blocking flush and there
> > 
> > 
> 
> Hi Fengguang,
> 
> I don't understand the purpose of this patch...
> 
> Currently, the value of 'ret' only affects the case where the commit
> exits early due to this being a non-blocking flush where we have not yet
> written back enough pages to make it worth our while to send a commit.
> 
> In essence, this really only matters for the cases where someone calls
> 'write_inode_now' (not used by anybody calling into the NFS client) and
> 'sync_inode', which is only called by nfs_wb_all (with sync_mode =
> WB_SYNC_ALL).
> 
> So can you please elaborate on the possible use cases for this change?

Yeah it has no real impact for current kernel. The "fix" is just to
make it behave more aligned to my expectation.

It did lead to a sync() hung bug with the v1 patch 4/6 in this series,
where I do the below code and expected "write_inode() == 0" to be
"done with the inode".  But only to find that it's not the case for NFS..

Thanks,
Fengguang
---

@@ -389,6 +389,8 @@ writeback_single_inode(struct inode *ino
                int err = write_inode(inode, wbc);
                if (ret == 0)
                        ret = err;
+               if (!err)
+                       wbc->inodes_written++;
        }
       
        spin_lock(&inode_lock);
@@ -664,6 +667,8 @@ static long wb_writeback(struct bdi_writ
                 */
                if (wbc.nr_to_write < MAX_WRITEBACK_PAGES)
                        continue;
+               if (wbc.inodes_written)
+                       continue;
               
                /*
                 * Nothing written and no more inodes for IO, bail

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/