diff mbox series

[01/14] namei: prepare do_rmdir for refactoring

Message ID 20210715103600.3570667-2-dkadashev@gmail.com (mailing list archive)
State New
Headers show
Series namei: clean up retry logic in various do_* functions | expand

Commit Message

Dmitry Kadashev July 15, 2021, 10:35 a.m. UTC
This is just a preparation for the move of the main rmdir logic to a
separate function to make the logic easier to follow.  This change
contains the flow changes so that the actual change to move the main
logic to a separate function does no change the flow at all.

Two changes here:

1. Previously on filename_parentat() error the function used to exit
immediately, and now it will check the return code to see if ESTALE
retry is appropriate. The filename_parentat() does its own retries on
ESTALE, but this extra check should be completely fine.

2. The retry_estale() check is wrapped in unlikely(). Some other places
already have that and overall it seems to make sense.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/io-uring/CAHk-=wh=cpt_tQCirzFZRPawRpbuFTZ2MxNpXiyUF+eBXF=+sw@mail.gmail.com/
Link: https://lore.kernel.org/io-uring/CAHk-=wjFd0qn6asio=zg7zUTRmSty_TpAEhnwym1Qb=wFgCKzA@mail.gmail.com/
Signed-off-by: Dmitry Kadashev <dkadashev@gmail.com>
---
 fs/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Al Viro July 15, 2021, 7:49 p.m. UTC | #1
On Thu, Jul 15, 2021 at 05:35:47PM +0700, Dmitry Kadashev wrote:
> This is just a preparation for the move of the main rmdir logic to a
> separate function to make the logic easier to follow.  This change
> contains the flow changes so that the actual change to move the main
> logic to a separate function does no change the flow at all.
> 
> Two changes here:
> 
> 1. Previously on filename_parentat() error the function used to exit
> immediately, and now it will check the return code to see if ESTALE
> retry is appropriate. The filename_parentat() does its own retries on
> ESTALE, but this extra check should be completely fine.
> 
> 2. The retry_estale() check is wrapped in unlikely(). Some other places
> already have that and overall it seems to make sense.

That's not the way to do it.

static inline bool
retry_estale(const long error, const unsigned int flags)
{
        return unlikely(error == -ESTALE && !(flags & LOOKUP_REVAL));
}

And strip the redundant unlikely in the callers.  Having that markup
in callers makes sense only when different callers have different
odds of positive result, which is very much not the case here.
Dmitry Kadashev July 20, 2021, 6:52 a.m. UTC | #2
On Fri, Jul 16, 2021 at 2:49 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Jul 15, 2021 at 05:35:47PM +0700, Dmitry Kadashev wrote:
> > This is just a preparation for the move of the main rmdir logic to a
> > separate function to make the logic easier to follow.  This change
> > contains the flow changes so that the actual change to move the main
> > logic to a separate function does no change the flow at all.
> >
> > Two changes here:
> >
> > 1. Previously on filename_parentat() error the function used to exit
> > immediately, and now it will check the return code to see if ESTALE
> > retry is appropriate. The filename_parentat() does its own retries on
> > ESTALE, but this extra check should be completely fine.
> >
> > 2. The retry_estale() check is wrapped in unlikely(). Some other places
> > already have that and overall it seems to make sense.
>
> That's not the way to do it.
>
> static inline bool
> retry_estale(const long error, const unsigned int flags)
> {
>         return unlikely(error == -ESTALE && !(flags & LOOKUP_REVAL));
> }
>
> And strip the redundant unlikely in the callers.  Having that markup
> in callers makes sense only when different callers have different
> odds of positive result, which is very much not the case here.

Yeah, I thought about this, but wasn't sure about interplay of
inline+[un]likely(). But I see that it's used quite a bit throughout the
kernel code so I suppose it's fine. I'll use that next time, thanks.


--
Dmitry Kadashev
diff mbox series

Patch

diff --git a/fs/namei.c b/fs/namei.c
index b5adfd4f7de6..99d5c3a4c12e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3998,11 +3998,11 @@  int do_rmdir(int dfd, struct filename *name)
 	mnt_drop_write(path.mnt);
 exit2:
 	path_put(&path);
-	if (retry_estale(error, lookup_flags)) {
+exit1:
+	if (unlikely(retry_estale(error, lookup_flags))) {
 		lookup_flags |= LOOKUP_REVAL;
 		goto retry;
 	}
-exit1:
 	putname(name);
 	return error;
 }