On Fri, 18 May 2012 11:14:38 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Fri, May 18, 2012 at 7:29 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > 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.
>
> Jeff, not clear yet. What I am trying to say is, if the reclaim fails,
> then, we can't reliably move the element to the end of the list
> since its links are not reliable after dropping the lock.
> So we do not know if the reclaim will succeedor fail, if it fails,
> we can't be sure that move will not oops and so we do not take
> a chance and move it before attempting reclaim.
>
The list links will be reliable as long as you hold a reference to the
file. So, you just need to make sure you move it to the end of the list
before you do the cifsFileInfo_put.
--
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
