[PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Shin'ichiro Kawasaki]

When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
left. This patch series have two fox patches for the stale page cache. Same
fix approach was used as blkdev_fallocate() [1].

[1] https://marc.info/?l=linux-block&m=163236463716836

Shin'ichiro Kawasaki (2):
  block: Hold invalidate_lock in BLKDISCARD ioctl
  block: Hold invalidate_lock in BLKZEROOUT ioctl

 block/ioctl.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

-- 
2.33.1

[PATCH 1/2] block: Hold invalidate_lock in BLKDISCARD ioctl

[Shin'ichiro Kawasaki]

When BLKDISCARD ioctl and data read race, the data read leaves stale
page cache. To avoid the stale page cache, hold invalidate_lock of the
block device file mapping. The stale page cache is observed when
blktests test case block/009 is repeated hundreds of times.

This patch can be applied back to the stable kernel version v5.15.y
with slight patch edit. Rework is required for older stable kernels.

Fixes: 351499a172c0 ("block: Invalidate cache on discard v2")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Cc: stable@vger.kernel.org # v5.15
---
 block/ioctl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index d6af0ac97e57..9fa87f64f703 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -113,6 +113,7 @@ static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
 	uint64_t range[2];
 	uint64_t start, len;
 	struct request_queue *q = bdev_get_queue(bdev);
+	struct inode *inode = bdev->bd_inode;
 	int err;
 
 	if (!(mode & FMODE_WRITE))
@@ -135,12 +136,17 @@ static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
 	if (start + len > bdev_nr_bytes(bdev))
 		return -EINVAL;
 
+	filemap_invalidate_lock(inode->i_mapping);
 	err = truncate_bdev_range(bdev, mode, start, start + len - 1);
 	if (err)
-		return err;
+		goto fail;
+
+	err = blkdev_issue_discard(bdev, start >> 9, len >> 9,
+				   GFP_KERNEL, flags);
 
-	return blkdev_issue_discard(bdev, start >> 9, len >> 9,
-				    GFP_KERNEL, flags);
+fail:
+	filemap_invalidate_unlock(inode->i_mapping);
+	return err;
 }
 
 static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
-- 
2.33.1

[PATCH 2/2] block: Hold invalidate_lock in BLKZEROOUT ioctl

[Shin'ichiro Kawasaki]

When BLKZEROOUT ioctl and data read race, the data read leaves stale
page cache. To avoid the stale page cache, hold invalidate_lock of the
block device file mapping. The stale page cache is observed when
blktests test case block/009 is modified to call "blkdiscard -z" command
and repeated hundreds of times.

This patch can be applied back to the stable kernel version v5.15.y.
Rework is required for older stable kernels.

Fixes: 22dd6d356628 ("block: invalidate the page cache when issuing BLKZEROOUT")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Cc: stable@vger.kernel.org # v5.15
---
 block/ioctl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/ioctl.c b/block/ioctl.c
index 9fa87f64f703..0a1d10ac2e1a 100644
--- a/block/ioctl.c
+++ b/block/ioctl.c
@@ -154,6 +154,7 @@ static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
 {
 	uint64_t range[2];
 	uint64_t start, end, len;
+	struct inode *inode = bdev->bd_inode;
 	int err;
 
 	if (!(mode & FMODE_WRITE))
@@ -176,12 +177,17 @@ static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
 		return -EINVAL;
 
 	/* Invalidate the page cache, including dirty pages */
+	filemap_invalidate_lock(inode->i_mapping);
 	err = truncate_bdev_range(bdev, mode, start, end);
 	if (err)
-		return err;
+		goto fail;
+
+	err = blkdev_issue_zeroout(bdev, start >> 9, len >> 9, GFP_KERNEL,
+				   BLKDEV_ZERO_NOUNMAP);
 
-	return blkdev_issue_zeroout(bdev, start >> 9, len >> 9, GFP_KERNEL,
-			BLKDEV_ZERO_NOUNMAP);
+fail:
+	filemap_invalidate_unlock(inode->i_mapping);
+	return err;
 }
 
 static int put_ushort(unsigned short __user *argp, unsigned short val)
-- 
2.33.1

Re: [PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Ming Lei]

On Tue, Nov 09, 2021 at 07:47:21PM +0900, Shin'ichiro Kawasaki wrote:
> When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
> left. This patch series have two fox patches for the stale page cache. Same
> fix approach was used as blkdev_fallocate() [1].
> 
> [1] https://marc.info/?l=linux-block&m=163236463716836
> 
> Shin'ichiro Kawasaki (2):
>   block: Hold invalidate_lock in BLKDISCARD ioctl
>   block: Hold invalidate_lock in BLKZEROOUT ioctl
> 
>  block/ioctl.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

Yeah, the discard ioctl needs such fixes too, seems it isn't triggered
in the test disk of my test VM when running block/009.

BTW, BLKRESETZONE may need the fix too.


Thanks,
Ming

Re: [PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Jan Kara]

On Tue 09-11-21 19:07:26, Ming Lei wrote:
> On Tue, Nov 09, 2021 at 07:47:21PM +0900, Shin'ichiro Kawasaki wrote:
> > When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
> > left. This patch series have two fox patches for the stale page cache. Same
> > fix approach was used as blkdev_fallocate() [1].
> > 
> > [1] https://marc.info/?l=linux-block&m=163236463716836
> > 
> > Shin'ichiro Kawasaki (2):
> >   block: Hold invalidate_lock in BLKDISCARD ioctl
> >   block: Hold invalidate_lock in BLKZEROOUT ioctl
> > 
> >  block/ioctl.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> Yeah, the discard ioctl needs such fixes too, seems it isn't triggered
> in the test disk of my test VM when running block/009.
> 
> BTW, BLKRESETZONE may need the fix too.

Yeah, it seems like that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] block: Hold invalidate_lock in BLKDISCARD ioctl

[Jan Kara]

On Tue 09-11-21 19:47:22, Shin'ichiro Kawasaki wrote:
> When BLKDISCARD ioctl and data read race, the data read leaves stale
> page cache. To avoid the stale page cache, hold invalidate_lock of the
> block device file mapping. The stale page cache is observed when
> blktests test case block/009 is repeated hundreds of times.
> 
> This patch can be applied back to the stable kernel version v5.15.y
> with slight patch edit. Rework is required for older stable kernels.
> 
> Fixes: 351499a172c0 ("block: Invalidate cache on discard v2")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Cc: stable@vger.kernel.org # v5.15

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/ioctl.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/block/ioctl.c b/block/ioctl.c
> index d6af0ac97e57..9fa87f64f703 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -113,6 +113,7 @@ static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
>  	uint64_t range[2];
>  	uint64_t start, len;
>  	struct request_queue *q = bdev_get_queue(bdev);
> +	struct inode *inode = bdev->bd_inode;
>  	int err;
>  
>  	if (!(mode & FMODE_WRITE))
> @@ -135,12 +136,17 @@ static int blk_ioctl_discard(struct block_device *bdev, fmode_t mode,
>  	if (start + len > bdev_nr_bytes(bdev))
>  		return -EINVAL;
>  
> +	filemap_invalidate_lock(inode->i_mapping);
>  	err = truncate_bdev_range(bdev, mode, start, start + len - 1);
>  	if (err)
> -		return err;
> +		goto fail;
> +
> +	err = blkdev_issue_discard(bdev, start >> 9, len >> 9,
> +				   GFP_KERNEL, flags);
>  
> -	return blkdev_issue_discard(bdev, start >> 9, len >> 9,
> -				    GFP_KERNEL, flags);
> +fail:
> +	filemap_invalidate_unlock(inode->i_mapping);
> +	return err;
>  }
>  
>  static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
> -- 
> 2.33.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] block: Hold invalidate_lock in BLKZEROOUT ioctl

[Jan Kara]

On Tue 09-11-21 19:47:23, Shin'ichiro Kawasaki wrote:
> When BLKZEROOUT ioctl and data read race, the data read leaves stale
> page cache. To avoid the stale page cache, hold invalidate_lock of the
> block device file mapping. The stale page cache is observed when
> blktests test case block/009 is modified to call "blkdiscard -z" command
> and repeated hundreds of times.
> 
> This patch can be applied back to the stable kernel version v5.15.y.
> Rework is required for older stable kernels.
> 
> Fixes: 22dd6d356628 ("block: invalidate the page cache when issuing BLKZEROOUT")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Cc: stable@vger.kernel.org # v5.15

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  block/ioctl.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 9fa87f64f703..0a1d10ac2e1a 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -154,6 +154,7 @@ static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
>  {
>  	uint64_t range[2];
>  	uint64_t start, end, len;
> +	struct inode *inode = bdev->bd_inode;
>  	int err;
>  
>  	if (!(mode & FMODE_WRITE))
> @@ -176,12 +177,17 @@ static int blk_ioctl_zeroout(struct block_device *bdev, fmode_t mode,
>  		return -EINVAL;
>  
>  	/* Invalidate the page cache, including dirty pages */
> +	filemap_invalidate_lock(inode->i_mapping);
>  	err = truncate_bdev_range(bdev, mode, start, end);
>  	if (err)
> -		return err;
> +		goto fail;
> +
> +	err = blkdev_issue_zeroout(bdev, start >> 9, len >> 9, GFP_KERNEL,
> +				   BLKDEV_ZERO_NOUNMAP);
>  
> -	return blkdev_issue_zeroout(bdev, start >> 9, len >> 9, GFP_KERNEL,
> -			BLKDEV_ZERO_NOUNMAP);
> +fail:
> +	filemap_invalidate_unlock(inode->i_mapping);
> +	return err;
>  }
>  
>  static int put_ushort(unsigned short __user *argp, unsigned short val)
> -- 
> 2.33.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Shinichiro Kawasaki]

On Nov 09, 2021 / 12:59, Jan Kara wrote:
> On Tue 09-11-21 19:07:26, Ming Lei wrote:
> > On Tue, Nov 09, 2021 at 07:47:21PM +0900, Shin'ichiro Kawasaki wrote:
> > > When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
> > > left. This patch series have two fox patches for the stale page cache. Same
> > > fix approach was used as blkdev_fallocate() [1].
> > > 
> > > [1] https://marc.info/?l=linux-block&m=163236463716836
> > > 
> > > Shin'ichiro Kawasaki (2):
> > >   block: Hold invalidate_lock in BLKDISCARD ioctl
> > >   block: Hold invalidate_lock in BLKZEROOUT ioctl
> > > 
> > >  block/ioctl.c | 24 ++++++++++++++++++------
> > >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> > Yeah, the discard ioctl needs such fixes too, seems it isn't triggered
> > in the test disk of my test VM when running block/009.
> > 
> > BTW, BLKRESETZONE may need the fix too.
> 
> Yeah, it seems like that.

Thanks for the comments. I'll work on BLKRESETZONE also.

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Jens Axboe]

On 11/9/21 3:47 AM, Shin'ichiro Kawasaki wrote:
> When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
> left. This patch series have two fox patches for the stale page cache. Same
> fix approach was used as blkdev_fallocate() [1].
> 
> [1] https://marc.info/?l=linux-block&m=163236463716836
> 
> Shin'ichiro Kawasaki (2):
>   block: Hold invalidate_lock in BLKDISCARD ioctl
>   block: Hold invalidate_lock in BLKZEROOUT ioctl
> 
>  block/ioctl.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 

Do you want to do a v2 with BLKRESETZONE added as well? I can do these
separately. but a bit awkward right now as my main block 5.16 branch
doesn't yet have the bdev size changes. I'll queue this up post flushing
out the remaining block bits for 5.16, if v2 with BLKRESETZONE happens
before that I'll just use that one.

-- 
Jens Axboe

Re: [PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Jens Axboe]

On Tue, 9 Nov 2021 19:47:21 +0900, Shin'ichiro Kawasaki wrote:
> When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
> left. This patch series have two fox patches for the stale page cache. Same
> fix approach was used as blkdev_fallocate() [1].
> 
> [1] https://marc.info/?l=linux-block&m=163236463716836
> 
> Shin'ichiro Kawasaki (2):
>   block: Hold invalidate_lock in BLKDISCARD ioctl
>   block: Hold invalidate_lock in BLKZEROOUT ioctl
> 
> [...]

Applied, thanks!

[1/2] block: Hold invalidate_lock in BLKDISCARD ioctl
      commit: f275c2aa4b47f3056acd182f2ff752cace21d8a5
[2/2] block: Hold invalidate_lock in BLKZEROOUT ioctl
      commit: 4b4d8b9375582b90f4fd9c708c739593e3a44946

Best regards,
-- 
Jens Axboe

Re: [PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Shinichiro Kawasaki]

On Nov 09, 2021 / 07:33, Jens Axboe wrote:
> On 11/9/21 3:47 AM, Shin'ichiro Kawasaki wrote:
> > When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
> > left. This patch series have two fox patches for the stale page cache. Same
> > fix approach was used as blkdev_fallocate() [1].
> > 
> > [1] https://marc.info/?l=linux-block&m=163236463716836
> > 
> > Shin'ichiro Kawasaki (2):
> >   block: Hold invalidate_lock in BLKDISCARD ioctl
> >   block: Hold invalidate_lock in BLKZEROOUT ioctl
> > 
> >  block/ioctl.c | 24 ++++++++++++++++++------
> >  1 file changed, 18 insertions(+), 6 deletions(-)
> > 
> 
> Do you want to do a v2 with BLKRESETZONE added as well? I can do these
> separately. but a bit awkward right now as my main block 5.16 branch
> doesn't yet have the bdev size changes. I'll queue this up post flushing
> out the remaining block bits for 5.16, if v2 with BLKRESETZONE happens
> before that I'll just use that one.

Let's separate the BLKRESETZONE patch. I'll need some more time to prepare it.

I saw the BLKDISCARD and BLKZEROOUT patches queued in the block-5.16 branch.
Thanks!

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Chaitanya Kulkarni]

Shinichiro,

On 11/9/2021 2:47 AM, Shin'ichiro Kawasaki wrote:
> External email: Use caution opening links or attachments
> 
> 
> When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
> left. This patch series have two fox patches for the stale page cache. Same
> fix approach was used as blkdev_fallocate() [1].
> 
> [1] https://marc.info/?l=linux-block&m=163236463716836

Thanks for the fixes, do we have blktest to validate this fix ?

Re: [PATCH 0/2] block: Fix stale page cache of discard or zero out ioctl

[Damien Le Moal]

On 2021/11/10 15:37, Chaitanya Kulkarni wrote:
> Shinichiro,
> 
> On 11/9/2021 2:47 AM, Shin'ichiro Kawasaki wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> When BLKDISCARD or BLKZEROOUT ioctl race with data read, stale page cache is
>> left. This patch series have two fox patches for the stale page cache. Same
>> fix approach was used as blkdev_fallocate() [1].
>>
>> [1] https://marc.info/?l=linux-block&m=163236463716836
> 
> Thanks for the fixes, do we have blktest to validate this fix ?

Yes. The problem was detected with block/009.

-- 
Damien Le Moal
Western Digital Research