From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J. Bruce Fields" Subject: Re: [PATCH v8 11/32] vfs: make do_unlinkat retry on ESTALE errors Date: Tue, 30 Oct 2012 15:28:09 -0400 Message-ID: <20121030192809.GE24618@fieldses.org> References: <1351341219-17837-1-git-send-email-jlayton@redhat.com> <1351341219-17837-12-git-send-email-jlayton@redhat.com> <20121030161429.GD24618@fieldses.org> <20121030123355.5c404373@tlielax.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, michael.brantley-Iq/kdjr4a97QT0dZR+AlfA@public.gmane.org, hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, miklos-sUDqSbJrdHQHWmgEVkV9KA@public.gmane.org, pstaubach-83r9SdEf25FBDgjK7y7TUQ@public.gmane.org To: Jeff Layton Return-path: Content-Disposition: inline In-Reply-To: <20121030123355.5c404373-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org> Sender: linux-nfs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org On Tue, Oct 30, 2012 at 12:33:55PM -0400, Jeff Layton wrote: > On Tue, 30 Oct 2012 12:14:29 -0400 > "J. Bruce Fields" wrote: > > > On Sat, Oct 27, 2012 at 08:33:18AM -0400, Jeff Layton wrote: > > > Signed-off-by: Jeff Layton > > > --- > > > fs/namei.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/namei.c b/fs/namei.c > > > index 7c9bb50..467b9f1 100644 > > > --- a/fs/namei.c > > > +++ b/fs/namei.c > > > @@ -3446,9 +3446,13 @@ static long do_unlinkat(int dfd, const char __user *pathname) > > > struct filename *name; > > > struct dentry *dentry; > > > struct nameidata nd; > > > - struct inode *inode = NULL; > > > + struct inode *inode; > > > + unsigned int try = 0; > > > + unsigned int lookup_flags = LOOKUP_PARENT; > > > > > > - name = user_path_parent(dfd, pathname, &nd, 0); > > > +retry: > > > + inode = NULL; > > > > So, you fail after "inode" was set (say vfs_unlink returned an error) > > the first time, then before "inode" was set (lookup_hash returns an > > error), and you end up incorrectly doing another iput() the second time > > through if you don't reset inode here? > > > > (I think I made the same mistake in another patch, actually....) > > > > --b. > > > > Correct. That's a new delta in this patch, btw. The original patch > didn't do that and it was causing a busy inodes on umount bug in > testing. > > It would occasionally hit an ESTALE error in this function and > because "inode" wasn't reset to NULL, it would do a double-put of the > inode and cause the counter to underflow. > > It might be good to restructure this code to make those sorts of bugs > less likely, but the error handling in here is already so hairy that I > decided to punt on that for now... Understood. I might find it just a little more obvious why we're doing this if the assignment was next to the final iput: if (inode) iput(inode); inode = NULL; ... --b. > > > > + name = user_path_parent(dfd, pathname, &nd, try); > > > if (IS_ERR(name)) > > > return PTR_ERR(name); > > > > > > @@ -3486,6 +3490,10 @@ exit2: > > > exit1: > > > path_put(&nd.path); > > > putname(name); > > > + if (retry_estale(error, try++)) { > > > + lookup_flags |= LOOKUP_REVAL; > > > + goto retry; > > > + } > > > return error; > > > > > > slashes: > > > -- > > > 1.7.11.7 > > > > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > Jeff Layton -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html