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: [email protected]
Reported-by: Darrick J. Wong <[email protected]>
Signed-off-by: Sarthak Kukreti <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
Signed-off-by: Mike Snitzer <[email protected]>
---
block/fops.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
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;
--
2.42.0.609.gbb76f46606-goog
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.
--
Jens Axboe
On Wed, Oct 11 2023 at 4:20P -0400,
Jens Axboe <[email protected]> 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
On 10/11/23 2:50 PM, Mike Snitzer wrote:
> On Wed, Oct 11 2023 at 4:20P -0400,
> Jens Axboe <[email protected]> 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.
--
Jens Axboe
On Wed, Oct 11 2023 at 4:53P -0400,
Jens Axboe <[email protected]> wrote:
> On 10/11/23 2:50 PM, Mike Snitzer wrote:
> > On Wed, Oct 11 2023 at 4:20P -0400,
> > Jens Axboe <[email protected]> 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.
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
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
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,
--
Jens Axboe