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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
