| Message ID | 20170414140753.16108-3-aryabinin@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14.04.2017 17:07, Andrey Ryabinin wrote: > invalidate_bdev() calls cleancache_invalidate_inode() iff ->nrpages != 0 > which doen't make any sense. > Make invalidate_bdev() always invalidate cleancache data. > > Fixes: c515e1fd361c ("mm/fs: add hooks to support cleancache") > Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> > --- > fs/block_dev.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/fs/block_dev.c b/fs/block_dev.c > index e405d8e..7af4787 100644 > --- a/fs/block_dev.c > +++ b/fs/block_dev.c > @@ -103,12 +103,11 @@ void invalidate_bdev(struct block_device *bdev) > { > struct address_space *mapping = bdev->bd_inode->i_mapping; > > - if (mapping->nrpages == 0) > - return; > - > - invalidate_bh_lrus(); > - lru_add_drain_all(); /* make sure all lru add caches are flushed */ > - invalidate_mapping_pages(mapping, 0, -1); > + if (mapping->nrpages) { > + invalidate_bh_lrus(); > + lru_add_drain_all(); /* make sure all lru add caches are flushed */ > + invalidate_mapping_pages(mapping, 0, -1); > + } How is this different than the current code? You will only invalidate the mapping iff ->nrpages > 0 ( I assume it can't go down below 0) ? Perhaps just remove the if altogether? > /* 99% of the time, we don't need to flush the cleancache on the bdev. > * But, for the strange corners, lets be cautious > */ > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/18/2017 09:51 PM, Nikolay Borisov wrote: > > > On 14.04.2017 17:07, Andrey Ryabinin wrote: >> invalidate_bdev() calls cleancache_invalidate_inode() iff ->nrpages != 0 >> which doen't make any sense. >> Make invalidate_bdev() always invalidate cleancache data. >> >> Fixes: c515e1fd361c ("mm/fs: add hooks to support cleancache") >> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> >> --- >> fs/block_dev.c | 11 +++++------ >> 1 file changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/fs/block_dev.c b/fs/block_dev.c >> index e405d8e..7af4787 100644 >> --- a/fs/block_dev.c >> +++ b/fs/block_dev.c >> @@ -103,12 +103,11 @@ void invalidate_bdev(struct block_device *bdev) >> { >> struct address_space *mapping = bdev->bd_inode->i_mapping; >> >> - if (mapping->nrpages == 0) >> - return; >> - >> - invalidate_bh_lrus(); >> - lru_add_drain_all(); /* make sure all lru add caches are flushed */ >> - invalidate_mapping_pages(mapping, 0, -1); >> + if (mapping->nrpages) { >> + invalidate_bh_lrus(); >> + lru_add_drain_all(); /* make sure all lru add caches are flushed */ >> + invalidate_mapping_pages(mapping, 0, -1); >> + } > > How is this different than the current code? You will only invalidate > the mapping iff ->nrpages > 0 ( I assume it can't go down below 0) ? The difference is that invalidate_bdev() now always calls cleancache_invalidate_inode() (you won't see it in this diff, it's placed after this if(mapping->nrpages){} block,) > Perhaps just remove the if altogether? > Given that invalidate_mapping_pages() invalidates exceptional entries as well, it certainly doesn't look right that we look only at mapping->nrpages and completely ignore ->nrexceptional. So maybe removing if() would be a right thing to do. But I think that should be a separate patch as it would fix a another bug probably introduced by commit 91b0abe36a7b ("mm + fs: store shadow entries in page cache") My intention here was to fix only cleancache case. >> /* 99% of the time, we don't need to flush the cleancache on the bdev. >> * But, for the strange corners, lets be cautious >> */ >> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/block_dev.c b/fs/block_dev.c index e405d8e..7af4787 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -103,12 +103,11 @@ void invalidate_bdev(struct block_device *bdev) { struct address_space *mapping = bdev->bd_inode->i_mapping; - if (mapping->nrpages == 0) - return; - - invalidate_bh_lrus(); - lru_add_drain_all(); /* make sure all lru add caches are flushed */ - invalidate_mapping_pages(mapping, 0, -1); + if (mapping->nrpages) { + invalidate_bh_lrus(); + lru_add_drain_all(); /* make sure all lru add caches are flushed */ + invalidate_mapping_pages(mapping, 0, -1); + } /* 99% of the time, we don't need to flush the cleancache on the bdev. * But, for the strange corners, lets be cautious */
invalidate_bdev() calls cleancache_invalidate_inode() iff ->nrpages != 0 which doen't make any sense. Make invalidate_bdev() always invalidate cleancache data. Fixes: c515e1fd361c ("mm/fs: add hooks to support cleancache") Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com> --- fs/block_dev.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)