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.
>
>> >
>> >> + ++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
