diff mbox series

block: Don't invalidate pagecache for invalid falloc modes

Message ID 20231011201230.750105-1-sarthakkukreti@chromium.org
State New
Headers
Series block: Don't invalidate pagecache for invalid falloc modes |

Commit Message

Sarthak Kukreti Oct. 11, 2023, 8:12 p.m. UTC 
  Only call truncate_bdev_range() if the fallocate mode is
supported. This fixes a bug where data in the pagecache
could be invalidated if the fallocate() was called on the
block device with an invalid mode.

Fixes: 25f4c41415e5 ("block: implement (some of) fallocate for block devices")
Cc: stable@vger.kernel.org
Reported-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Sarthak Kukreti <sarthakkukreti@chromium.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 block/fops.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Comments

Jens Axboe Oct. 11, 2023, 8:20 p.m. UTC | #1 
On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
> Only call truncate_bdev_range() if the fallocate mode is
> supported. This fixes a bug where data in the pagecache
> could be invalidated if the fallocate() was called on the
> block device with an invalid mode.

Fix looks fine, but would be nicer if we didn't have to duplicate the
truncate_bdev_range() in each switch clause. Can we check this upfront
instead?

Also, please wrap commit messages at 72-74 chars.
Mike Snitzer Oct. 11, 2023, 8:50 p.m. UTC | #2 
On Wed, Oct 11 2023 at  4:20P -0400,
Jens Axboe <axboe@kernel.dk> wrote:

> On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
> > Only call truncate_bdev_range() if the fallocate mode is
> > supported. This fixes a bug where data in the pagecache
> > could be invalidated if the fallocate() was called on the
> > block device with an invalid mode.
> 
> Fix looks fine, but would be nicer if we didn't have to duplicate the
> truncate_bdev_range() in each switch clause. Can we check this upfront
> instead?

No, if you look at the function (rather than just the patch in
isolation) we need to make the call for each case rather than collapse
to a single call at the front (that's the reason for this fix, because
otherwise the default: error case will invalidate the page cache too).

Just so you're aware, I also had this feedback that shaped the patch a
bit back in April:
https://listman.redhat.com/archives/dm-devel/2023-April/053986.html

> Also, please wrap commit messages at 72-74 chars.

Not seeing where the header should be wrapped.  You referring to the
Fixes: line?  I've never seen those wrapped.

Mike
Jens Axboe Oct. 11, 2023, 8:53 p.m. UTC | #3 
On 10/11/23 2:50 PM, Mike Snitzer wrote:
> On Wed, Oct 11 2023 at  4:20P -0400,
> Jens Axboe <axboe@kernel.dk> wrote:
> 
>> On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
>>> Only call truncate_bdev_range() if the fallocate mode is
>>> supported. This fixes a bug where data in the pagecache
>>> could be invalidated if the fallocate() was called on the
>>> block device with an invalid mode.
>>
>> Fix looks fine, but would be nicer if we didn't have to duplicate the
>> truncate_bdev_range() in each switch clause. Can we check this upfront
>> instead?
> 
> No, if you look at the function (rather than just the patch in
> isolation) we need to make the call for each case rather than collapse
> to a single call at the front (that's the reason for this fix, because
> otherwise the default: error case will invalidate the page cache too).

Yes that part is clear, but it might look cleaner to check a valid mask
first rather than have 3 duplicate calls.

> Just so you're aware, I also had this feedback that shaped the patch a
> bit back in April:
> https://listman.redhat.com/archives/dm-devel/2023-April/053986.html
> 
>> Also, please wrap commit messages at 72-74 chars.
> 
> Not seeing where the header should be wrapped.  You referring to the
> Fixes: line?  I've never seen those wrapped.

I'm referring to the commit message itself.
Mike Snitzer Oct. 11, 2023, 9:08 p.m. UTC | #4 
On Wed, Oct 11 2023 at  4:53P -0400,
Jens Axboe <axboe@kernel.dk> wrote:

> On 10/11/23 2:50 PM, Mike Snitzer wrote:
> > On Wed, Oct 11 2023 at  4:20P -0400,
> > Jens Axboe <axboe@kernel.dk> wrote:
> > 
> >> On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
> >>> Only call truncate_bdev_range() if the fallocate mode is
> >>> supported. This fixes a bug where data in the pagecache
> >>> could be invalidated if the fallocate() was called on the
> >>> block device with an invalid mode.
> >>
> >> Fix looks fine, but would be nicer if we didn't have to duplicate the
> >> truncate_bdev_range() in each switch clause. Can we check this upfront
> >> instead?
> > 
> > No, if you look at the function (rather than just the patch in
> > isolation) we need to make the call for each case rather than collapse
> > to a single call at the front (that's the reason for this fix, because
> > otherwise the default: error case will invalidate the page cache too).
> 
> Yes that part is clear, but it might look cleaner to check a valid mask
> first rather than have 3 duplicate calls.

OK.
 
> > Just so you're aware, I also had this feedback that shaped the patch a
> > bit back in April:
> > https://listman.redhat.com/archives/dm-devel/2023-April/053986.html
> > 
> >> Also, please wrap commit messages at 72-74 chars.
> > 
> > Not seeing where the header should be wrapped.  You referring to the
> > Fixes: line?  I've never seen those wrapped.
> 
> I'm referring to the commit message itself.

Ah, you'd like lines extended because they are too short.
Jens Axboe Oct. 11, 2023, 9:20 p.m. UTC | #5 
On 10/11/23 3:08 PM, Mike Snitzer wrote:
>>>> Also, please wrap commit messages at 72-74 chars.
>>>
>>> Not seeing where the header should be wrapped.  You referring to the
>>> Fixes: line?  I've never seen those wrapped.
>>
>> I'm referring to the commit message itself.
> 
> Ah, you'd like lines extended because they are too short.

Exactly, it's way too short.
Jens Axboe Oct. 11, 2023, 9:54 p.m. UTC | #6 
On 10/11/23 2:20 PM, Jens Axboe wrote:
> On 10/11/23 2:12 PM, Sarthak Kukreti wrote:
>> Only call truncate_bdev_range() if the fallocate mode is
>> supported. This fixes a bug where data in the pagecache
>> could be invalidated if the fallocate() was called on the
>> block device with an invalid mode.
> 
> Fix looks fine, but would be nicer if we didn't have to duplicate the
> truncate_bdev_range() in each switch clause. Can we check this upfront
> instead?

Don't see a good way to do it on my end, so let's just go with what is
there now. I applied it with the commit message reformatted.
Jens Axboe Oct. 11, 2023, 9:54 p.m. UTC | #7 
On Wed, 11 Oct 2023 13:12:30 -0700, Sarthak Kukreti wrote:
> Only call truncate_bdev_range() if the fallocate mode is
> supported. This fixes a bug where data in the pagecache
> could be invalidated if the fallocate() was called on the
> block device with an invalid mode.
> 
> 

Applied, thanks!

[1/1] block: Don't invalidate pagecache for invalid falloc modes
      commit: 1364a3c391aedfeb32aa025303ead3d7c91cdf9d

Best regards,
diff mbox series

Patch

diff --git a/block/fops.c b/block/fops.c
index acff3d5d22d4..73e42742543f 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -772,24 +772,35 @@  static long blkdev_fallocate(struct file *file, int mode, loff_t start,
 
 	filemap_invalidate_lock(inode->i_mapping);
 
-	/* Invalidate the page cache, including dirty pages. */
-	error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end);
-	if (error)
-		goto fail;
-
+	/*
+	 * Invalidate the page cache, including dirty pages, for valid
+	 * de-allocate mode calls to fallocate().
+	 */
 	switch (mode) {
 	case FALLOC_FL_ZERO_RANGE:
 	case FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE:
+		error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end);
+		if (error)
+			goto fail;
+
 		error = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT,
 					     len >> SECTOR_SHIFT, GFP_KERNEL,
 					     BLKDEV_ZERO_NOUNMAP);
 		break;
 	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE:
+		error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end);
+		if (error)
+			goto fail;
+
 		error = blkdev_issue_zeroout(bdev, start >> SECTOR_SHIFT,
 					     len >> SECTOR_SHIFT, GFP_KERNEL,
 					     BLKDEV_ZERO_NOFALLBACK);
 		break;
 	case FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE | FALLOC_FL_NO_HIDE_STALE:
+		error = truncate_bdev_range(bdev, file_to_blk_mode(file), start, end);
+		if (error)
+			goto fail;
+
 		error = blkdev_issue_discard(bdev, start >> SECTOR_SHIFT,
 					     len >> SECTOR_SHIFT, GFP_KERNEL);
 		break;