| Message ID | 20210715103600.3570667-2-dkadashev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | namei: clean up retry logic in various do_* functions | expand |
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.
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 --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; }
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(-)