| Message ID | 20231011201230.750105-1-sarthakkukreti@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Don't invalidate pagecache for invalid falloc modes | expand |
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.
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
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.
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.
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.
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.
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 --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;
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(-)