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
