From patchwork Tue Aug 29 21:37:32 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bruce Fields X-Patchwork-Id: 9928383 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2DAB360380 for ; Tue, 29 Aug 2017 21:37:37 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1E65828A5A for ; Tue, 29 Aug 2017 21:37:37 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 12BB928A81; Tue, 29 Aug 2017 21:37:37 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A999D28A5A for ; Tue, 29 Aug 2017 21:37:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751463AbdH2Vhe (ORCPT ); Tue, 29 Aug 2017 17:37:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49342 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751272AbdH2Vhd (ORCPT ); Tue, 29 Aug 2017 17:37:33 -0400 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 750F07E42F; Tue, 29 Aug 2017 21:37:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 750F07E42F Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=bfields@redhat.com Received: from parsley.fieldses.org (ovpn-117-61.phx2.redhat.com [10.3.117.61]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3AA436EC61; Tue, 29 Aug 2017 21:37:33 +0000 (UTC) Received: by parsley.fieldses.org (Postfix, from userid 2815) id 32D2B180D65; Tue, 29 Aug 2017 17:37:32 -0400 (EDT) Date: Tue, 29 Aug 2017 17:37:32 -0400 From: "J. Bruce Fields" To: NeilBrown Cc: linux-nfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, Trond Myklebust Subject: Re: [PATCH 1/3] fs: cleanup to hide some details of delegation logic Message-ID: <20170829213731.GH8822@parsley.fieldses.org> References: <1503697958-6122-1-git-send-email-bfields@redhat.com> <1503697958-6122-2-git-send-email-bfields@redhat.com> <87efrwibyz.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87efrwibyz.fsf@notabene.neil.brown.name> User-Agent: Mutt/1.8.3 (2017-05-23) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 29 Aug 2017 21:37:33 +0000 (UTC) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Aug 28, 2017 at 01:54:12PM +1000, NeilBrown wrote: > On Fri, Aug 25 2017, J. Bruce Fields wrote: > > > From: "J. Bruce Fields" > > > > Pull the checks for delegated_inode into break_deleg_wait() to simplify > > the callers a little. > > > > No change in behavior. > > > > Signed-off-by: J. Bruce Fields > > --- > > fs/namei.c | 26 ++++++++++---------------- > > fs/open.c | 16 ++++++---------- > > fs/utimes.c | 8 +++----- > > include/linux/fs.h | 12 +++++++----- > > 4 files changed, 26 insertions(+), 36 deletions(-) > > > > diff --git a/fs/namei.c b/fs/namei.c > > index ddb6a7c2b3d4..5a93be7b2c9c 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -4048,11 +4048,9 @@ static long do_unlinkat(int dfd, const char __user *pathname) > > if (inode) > > iput(inode); /* truncate the inode here */ > > inode = NULL; > > - if (delegated_inode) { > > - error = break_deleg_wait(&delegated_inode); > > - if (!error) > > - goto retry_deleg; > > - } > > + error = break_deleg_wait(&delegated_inode, error); > > + if (error == DELEG_RETRY) > > + goto retry_deleg; > > > > I don't like the "DELEG_RETRY". You are comparing it against an > 'error', but it doesn't start with '-E', so I get confused (happens > often). > > If this read: > > if (error > 0) > goto retry_deleg; > > it would be must more obvious to me what was happening. Clearly the > return value isn't an error, and it isn't "success" either. This is a > pattern I've seen elsewhere. > > Alternately you could use "-EAGAIN", but I suspect there is a risk of > unwanted side-effects if you re-use and existing code. Yes. OK, I think I like your suggestion. The change would look like the following (untested). --b. diff --git a/fs/namei.c b/fs/namei.c index 5a93be7b2c9c..e8688498aff7 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -4049,7 +4049,7 @@ static long do_unlinkat(int dfd, const char __user *pathname) iput(inode); /* truncate the inode here */ inode = NULL; error = break_deleg_wait(&delegated_inode, error); - if (error == DELEG_RETRY) + if (error > 0) goto retry_deleg; mnt_drop_write(path.mnt); exit1: @@ -4282,7 +4282,7 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname, out_dput: done_path_create(&new_path, new_dentry); error = break_deleg_wait(&delegated_inode, error); - if (error == DELEG_RETRY) { + if (error > 0) { path_put(&old_path); goto retry; } @@ -4598,7 +4598,7 @@ SYSCALL_DEFINE5(renameat2, int, olddfd, const char __user *, oldname, exit3: unlock_rename(new_path.dentry, old_path.dentry); error = break_deleg_wait(&delegated_inode, error); - if (error == DELEG_RETRY) + if (error > 0) goto retry_deleg; mnt_drop_write(old_path.mnt); exit2: diff --git a/fs/open.c b/fs/open.c index d49e9385e45d..80975c4dd146 100644 --- a/fs/open.c +++ b/fs/open.c @@ -533,7 +533,7 @@ static int chmod_common(const struct path *path, umode_t mode) out_unlock: inode_unlock(inode); error = break_deleg_wait(&delegated_inode, error); - if (error == DELEG_RETRY) + if (error > 0) goto retry_deleg; mnt_drop_write(path->mnt); return error; @@ -610,7 +610,7 @@ static int chown_common(const struct path *path, uid_t user, gid_t group) error = notify_change(path->dentry, &newattrs, &delegated_inode); inode_unlock(inode); error = break_deleg_wait(&delegated_inode, error); - if (error == DELEG_RETRY) + if (error > 0) goto retry_deleg; return error; } diff --git a/fs/utimes.c b/fs/utimes.c index 75467b7ebfce..4dc6717638e6 100644 --- a/fs/utimes.c +++ b/fs/utimes.c @@ -90,7 +90,7 @@ static int utimes_common(const struct path *path, struct timespec *times) error = notify_change(path->dentry, &newattrs, &delegated_inode); inode_unlock(inode); error = break_deleg_wait(&delegated_inode, error); - if (error == DELEG_RETRY) + if (error > 0) goto retry_deleg; mnt_drop_write(path->mnt); diff --git a/include/linux/fs.h b/include/linux/fs.h index 1d0d2fde1766..1c7f7be3f26d 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2288,8 +2288,6 @@ static inline int try_break_deleg(struct inode *inode, struct inode **delegated_ return ret; } -#define DELEG_RETRY 1 - static inline int break_deleg_wait(struct inode **delegated_inode, int error) { if (!*delegated_inode) @@ -2297,7 +2295,13 @@ static inline int break_deleg_wait(struct inode **delegated_inode, int error) error = break_deleg(*delegated_inode, O_WRONLY); iput(*delegated_inode); *delegated_inode = NULL; - return error ? error : DELEG_RETRY; + if (error) + return error; + /* + * Signal to the caller that it can retry the original operation + * now that the delegation is broken: + */ + return 1; } static inline int break_layout(struct inode *inode, bool wait)