On Wed, 16 May 2012 15:41:24 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
[...]
> >
> > Hmm...now that I think about this, do we really want to move this to
> > the end of the list here? If this turns out to be reclaimable we will
> > have moved it to the end of the list, and then it will be the *last*
> > one found on the next pass into this function.
>
> yes, but we need to keep this element on the list while reclaimation
> is going on. If we keep where it is and reclaim fails, we may oops
> for we are not sure if the next element is still intact (that is the reason
> of the oops in the first place) since we will continue down the list
> looking for another file handle.
>
No, because once you drop the spinlock to do the reclaim you have to
start walking the list from the head again.
> >
> >> spin_unlock(&cifs_file_list_lock);
> >> +
> >> + if (inv_file) {
> >> + rc = cifs_reopen_file(inv_file, false);
> >> + if (!rc)
> >> + return inv_file;
> >> + else {
> >> + spin_lock(&cifs_file_list_lock);
> >> + cifsFileInfo_put(inv_file);
> >
> > Here's where I have to rescind my Ack -- sorry :(
> >
> > This is going to deadlock -- the first thing
> > that cifsFileInfo_put() does is lock the
> > cifs_file_list_lock.
>
> Agree. Thanks. I think reversing the order of the calls
> spin_lock() and cifsFileInfo_put(), should take care of that.
>
Yes, that should be fine.
> >
> > It seems to me that moving the file to the end of the
> > list here would make more sense. IOW, we should only do
> > that if it turned out not to be reclaimable. Right?
>
> But the problem would remain. We are reclaiming without lock
> so we have to consider the possibility that reclaim could fail, so
> list move could fail if the next element has become stale/invalid.
>
No -- if the reclaim fails, you go to refind_writable and start
walking the list all over again from the head. Once you drop the
spinlock you *must* do that regardless of whether you moved this entry
or not. You don't know if other threads have modified it while you no
longer hold the lock.
The idea with moving this to the end is that you want to avoid picking
the same FID over and over again in the event that it's unreclaimable,
while there are others that could be reclaimed. There's no need to do
that however if the file is able to be reclaimed.
> >
> >> + ++refind;
> >> + goto refind_writable;
> >> + }
> >> + }
> >> +
> >> return NULL;
> >> }
> >>
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
