linux-kernel - Re: [PATCH 6/6] NFS: return -EAGAIN when skipped commit in nfs_commit_unstable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 Apr 2011 11:55:04 +0800
From:	Wu Fengguang <fengguang.wu@...el.com>
To:	Trond Myklebust <Trond.Myklebust@...app.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>, Jan Kara <jack@...e.cz>,
	Mel Gorman <mel@...ux.vnet.ibm.com>,
	Dave Chinner <david@...morbit.com>,
	Itaru Kitayama <kitayama@...bb4u.ne.jp>,
	Minchan Kim <minchan.kim@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	Linux Memory Management List <linux-mm@...ck.org>
Subject: Re: [PATCH 6/6] NFS: return -EAGAIN when skipped commit in
 nfs_commit_unstable_pages()

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@...app.com>
> > Signed-off-by: Wu Fengguang <fengguang.wu@...el.com>
> > ---
> >  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/