Re: [PATCH 2/4] fs/block_dev: always invalidate cleancache in invalidate_bdev()
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>> --- >> 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 >> */ >>
Re: [PATCH 2/4] fs/block_dev: always invalidate cleancache in invalidate_bdev()
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 >> --- >> 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 >> */ >>
Re: [PATCH 2/4] fs/block_dev: always invalidate cleancache in invalidate_bdev()
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> --- > 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 >*/ >
Re: [PATCH 2/4] fs/block_dev: always invalidate cleancache in invalidate_bdev()
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 > --- > 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 >*/ >