Re: [PATCH 1/2] NFS: Fix a bug in nfs_fscache_release_page()

[Suresh Jayaraman <sjayaraman@suse.de>]

On 02/08/2010 08:20 PM, Trond Myklebust wrote:
> On Mon, 2010-02-08 at 08:23 -0500, Trond Myklebust wrote: 
>> On Sun, 2010-02-07 at 16:56 +0530, Suresh Jayaraman wrote: 
>>> There are only two callers for nfs_fscache_release_page() -
>>> nfs_release_page() and nfs_migrate_page(). nfs_migrate_page already does
>>> this:
>>>
>>>        if (PageFsCache(page))
>>>                 nfs_fscache_release_page(page, GFP_KERNEL);
>>>
>>> and the assumption in nfs_release_page() is that the page should have
>>> either PG_private set or PG_fscache set and nfs_fscache_release_page
>>> gets called only if PG_private is not set.
>>
>> ...or if it gets cleared.
> 
> To be more precise, even before we put call to nfs_wb_page() in
> nfs_release_page(), it was possible for the PG_private bit to be set

Yes, I have seen a similar bug report before we added nfs_wb_page on a
2.6.32 kernel too.

> when doing the test in shrink_page_list(), but for an outstanding commit
> operation to complete before the second test in nfs_release_page.
> 
> In this case, nfs_fscache_release_page would get called with neither
> PG_private nor PG_fscache being set, and the Oops could occur.
> 

We seem to ensure that we're holding a page lock in try_to_release_page.
Even if the outstanding commit is complete by the time we are in
nfs_releage_page, page flags should not have been modified, right?

>>> I think the idea is that nfs_fscache_release_page should not get called
>>> if fsc option is not used. So it appears to me this patch is fixing the
>>> symptom not the actual issue. Perhaps, this the assumption in
>>> nfs_release_page is wrong or the PageFsCache() check should be moved to
>>> nfs_release_page?
>>
>> No. We should rather get rid of the redundant check for PageFsCache() in
>> nfs_migrate_page. PageFsCache() is particular to fscache, so the test
>> belongs in the fscache code.

yes, make sense.

Thanks,

-- 
Suresh Jayaraman