* [PATCH] block: Don't invalidate pagecache for invalid falloc modes
@ 2023-10-11 20:12 Sarthak Kukreti
2023-10-11 20:20 ` Jens Axboe
2023-10-11 21:54 ` Jens Axboe
0 siblings, 2 replies; 8+ messages in thread
From: Sarthak Kukreti @ 2023-10-11 20:12 UTC (permalink / raw)
To: linux-block, linux-kernel
Cc: Bart Van Assche, Darrick J. Wong, Jens Axboe, Sarthak Kukreti,
stable, Darrick J . Wong, Christoph Hellwig, Mike Snitzer
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(-)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] block: Don't invalidate pagecache for invalid falloc modes
2023-10-11 20:12 [PATCH] block: Don't invalidate pagecache for invalid falloc modes Sarthak Kukreti
@ 2023-10-11 20:20 ` Jens Axboe
2023-10-11 20:50 ` Mike Snitzer
2023-10-11 21:54 ` [PATCH] " Jens Axboe
2023-10-11 21:54 ` Jens Axboe
1 sibling, 2 replies; 8+ messages in thread
From: Jens Axboe @ 2023-10-11 20:20 UTC (permalink / raw)
To: Sarthak Kukreti, linux-block, linux-kernel
Cc: Bart Van Assche, Darrick J. Wong, stable, Darrick J . Wong,
Christoph Hellwig, Mike Snitzer
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: block: Don't invalidate pagecache for invalid falloc modes
2023-10-11 20:20 ` Jens Axboe
@ 2023-10-11 20:50 ` Mike Snitzer
2023-10-11 20:53 ` Jens Axboe
2023-10-11 21:54 ` [PATCH] " Jens Axboe
1 sibling, 1 reply; 8+ messages in thread
From: Mike Snitzer @ 2023-10-11 20:50 UTC (permalink / raw)
To: Jens Axboe
Cc: Sarthak Kukreti, linux-block, linux-kernel, Bart Van Assche,
Darrick J. Wong, stable, Darrick J . Wong, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: block: Don't invalidate pagecache for invalid falloc modes
2023-10-11 20:50 ` Mike Snitzer
@ 2023-10-11 20:53 ` Jens Axboe
2023-10-11 21:08 ` Mike Snitzer
0 siblings, 1 reply; 8+ messages in thread
From: Jens Axboe @ 2023-10-11 20:53 UTC (permalink / raw)
To: Mike Snitzer
Cc: Sarthak Kukreti, linux-block, linux-kernel, Bart Van Assche,
Darrick J. Wong, stable, Darrick J . Wong, Christoph Hellwig
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.
--
Jens Axboe
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: block: Don't invalidate pagecache for invalid falloc modes
2023-10-11 20:53 ` Jens Axboe
@ 2023-10-11 21:08 ` Mike Snitzer
2023-10-11 21:20 ` Jens Axboe
0 siblings, 1 reply; 8+ messages in thread
From: Mike Snitzer @ 2023-10-11 21:08 UTC (permalink / raw)
To: Jens Axboe
Cc: Sarthak Kukreti, linux-block, linux-kernel, Bart Van Assche,
Darrick J. Wong, stable, Darrick J . Wong, Christoph Hellwig
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: block: Don't invalidate pagecache for invalid falloc modes
2023-10-11 21:08 ` Mike Snitzer
@ 2023-10-11 21:20 ` Jens Axboe
0 siblings, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2023-10-11 21:20 UTC (permalink / raw)
To: Mike Snitzer
Cc: Sarthak Kukreti, linux-block, linux-kernel, Bart Van Assche,
Darrick J. Wong, stable, Darrick J . Wong, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] block: Don't invalidate pagecache for invalid falloc modes
2023-10-11 20:20 ` Jens Axboe
2023-10-11 20:50 ` Mike Snitzer
@ 2023-10-11 21:54 ` Jens Axboe
1 sibling, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2023-10-11 21:54 UTC (permalink / raw)
To: Sarthak Kukreti, linux-block, linux-kernel
Cc: Bart Van Assche, Darrick J. Wong, stable, Darrick J . Wong,
Christoph Hellwig, Mike Snitzer
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] block: Don't invalidate pagecache for invalid falloc modes
2023-10-11 20:12 [PATCH] block: Don't invalidate pagecache for invalid falloc modes Sarthak Kukreti
2023-10-11 20:20 ` Jens Axboe
@ 2023-10-11 21:54 ` Jens Axboe
1 sibling, 0 replies; 8+ messages in thread
From: Jens Axboe @ 2023-10-11 21:54 UTC (permalink / raw)
To: linux-block, linux-kernel, Sarthak Kukreti
Cc: Bart Van Assche, Darrick J. Wong, stable, Darrick J . Wong,
Christoph Hellwig, Mike Snitzer
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-10-11 21:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-11 20:12 [PATCH] block: Don't invalidate pagecache for invalid falloc modes Sarthak Kukreti
2023-10-11 20:20 ` Jens Axboe
2023-10-11 20:50 ` Mike Snitzer
2023-10-11 20:53 ` Jens Axboe
2023-10-11 21:08 ` Mike Snitzer
2023-10-11 21:20 ` Jens Axboe
2023-10-11 21:54 ` [PATCH] " Jens Axboe
2023-10-11 21:54 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).