On Fri, Jul 16, 2021 at 2:49 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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
