Re: [PATCH 3/3] 9pfs: Improve unreclaim loop

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 21. Januar 2021 17:34:55 CET Greg Kurz wrote:
> > > +
> > > +    /*
> > > +     * v9fs_reopen_fid() can yield : a reference on the fid must be
> > > held
> > > +     * to ensure its pointer remains valid and we can safely pass it to
> > > +     * QSIMPLEQ_NEXT(). The corresponding put_fid() can also yield so
> > > +     * we must keep a reference on the next fid as well. So the logic
> > > here
> > > +     * is to get a reference on a fid and only put it back during the
> > > next
> > > +     * iteration after we could get a reference on the next fid. Start
> > > with +     * the first one.
> > > +     */
> > > +    for (fidp->ref++; fidp; fidp = fidp_next) {
> > > +        if (fidp->path.size == path->size &&
> > > +            !memcmp(fidp->path.data, path->data, path->size)) {
> > > 
> > >              /* Mark the fid non reclaimable. */
> > >              fidp->flags |= FID_NON_RECLAIMABLE;
> > >              
> > >              /* reopen the file/dir if already closed */
> > >              err = v9fs_reopen_fid(pdu, fidp);
> > >              if (err < 0) {
> > > 
> > > +                put_fid(pdu, fidp);
> > > 
> > >                  return err;
> > >              
> > >              }
> > > 
> > > +        }
> > > +
> > > +        fidp_next = QSIMPLEQ_NEXT(fidp, next);
> > > +
> > > +        if (fidp_next) {
> > > 
> > >              /*
> > > 
> > > -             * Go back to head of fid list because
> > > -             * the list could have got updated when
> > > -             * switched to the worker thread
> > > +             * Ensure the next fid survives a potential clunk request
> > > during +             * put_fid() below and v9fs_reopen_fid() in the next
> > > iteration. */
> > > -            if (err == 0) {
> > > -                goto again;
> > > -            }
> > > +            fidp_next->ref++;
> > 
> > Mmm, that works as intended if fidp_next matches the requested path.
> > However if it is not (like it would in the majority of cases) then the
> > loop breaks next and the bumped reference count would never be reverted.
> > Or am I missing something here?
> 
> Each iteration of the loop starts with an already referenced fidp.
> 
> The loop can only break if:
> 
> - v9fs_reopen_fid() fails, in which case put_fid(pdu, fidp) is called
>   on the error path above
> - end of list is reached, in which case the reference on fidp is
>   dropped just like in all previous iterations...

Ah yes, you're right. It's fine!

So just the  assert(fidp);  change then, and the log comment fixes would be 
nice to have. ;-) That's it.

> > >          }
> > > 
> > > +
> > > +        /* We're done with this fid */
> > > +        put_fid(pdu, fidp);
> 
> ... here.
> 
> > >      }
> > > 
> > > +
> > > 
> > >      return 0;
> > >  
> > >  }
> > 
> > Best regards,
> > Christian Schoenebeck

Best regards,
Christian Schoenebeck