| Message ID | 20230103104430.27749-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: don't allocate blocks beyond EOF from __mpage_writepage | expand |
On Tue, Jan 03, 2023 at 11:44:30AM +0100, Jan Kara wrote: > When __mpage_writepage() is called for a page beyond EOF, it will go and > allocate all blocks underlying the page. This is not only unnecessary > but this way blocks can get leaked (e.g. if a page beyond EOF is marked > dirty but in the end write fails and i_size is not extended). > > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/mpage.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/mpage.c b/fs/mpage.c > index 0f8ae954a579..9f040c1d5912 100644 > --- a/fs/mpage.c > +++ b/fs/mpage.c > @@ -524,6 +524,12 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc, > */ > BUG_ON(!PageUptodate(page)); > block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits); > + /* > + * Whole page beyond EOF? Skip allocating blocks to avoid leaking > + * space. > + */ > + if (block_in_file >= (i_size + (1 << blkbits) - 1) >> blkbits) > + goto page_is_mapped; > last_block = (i_size - 1) >> blkbits; Why not simply if (block_in_file > last_block) goto page_is_mapped; after last_block has been calculated?
On Wed 04-01-23 00:02:31, Al Viro wrote: > On Tue, Jan 03, 2023 at 11:44:30AM +0100, Jan Kara wrote: > > When __mpage_writepage() is called for a page beyond EOF, it will go and > > allocate all blocks underlying the page. This is not only unnecessary > > but this way blocks can get leaked (e.g. if a page beyond EOF is marked > > dirty but in the end write fails and i_size is not extended). > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/mpage.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/fs/mpage.c b/fs/mpage.c > > index 0f8ae954a579..9f040c1d5912 100644 > > --- a/fs/mpage.c > > +++ b/fs/mpage.c > > @@ -524,6 +524,12 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc, > > */ > > BUG_ON(!PageUptodate(page)); > > block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits); > > + /* > > + * Whole page beyond EOF? Skip allocating blocks to avoid leaking > > + * space. > > + */ > > + if (block_in_file >= (i_size + (1 << blkbits) - 1) >> blkbits) > > + goto page_is_mapped; > > last_block = (i_size - 1) >> blkbits; > > Why not simply > > if (block_in_file > last_block) > goto page_is_mapped; > > after last_block has been calculated? Because if i_size == 0, last_block is (~0 >> blkbits) (which was actually the case the test hit). Honza
On Tue, Jan 03, 2023 at 11:44:30AM +0100, Jan Kara wrote: > When __mpage_writepage() is called for a page beyond EOF, it will go and > allocate all blocks underlying the page. This is not only unnecessary > but this way blocks can get leaked (e.g. if a page beyond EOF is marked > dirty but in the end write fails and i_size is not extended). Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Sun 08-01-23 09:25:10, Christoph Hellwig wrote: > On Tue, Jan 03, 2023 at 11:44:30AM +0100, Jan Kara wrote: > > When __mpage_writepage() is called for a page beyond EOF, it will go and > > allocate all blocks underlying the page. This is not only unnecessary > > but this way blocks can get leaked (e.g. if a page beyond EOF is marked > > dirty but in the end write fails and i_size is not extended). > > Looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Matthew, Andrew, can one of you please pick up this fix? Thanks! Honza
On Wed, Jan 25, 2023 at 03:23:51PM +0100, Jan Kara wrote: > On Sun 08-01-23 09:25:10, Christoph Hellwig wrote: > > On Tue, Jan 03, 2023 at 11:44:30AM +0100, Jan Kara wrote: > > > When __mpage_writepage() is called for a page beyond EOF, it will go and > > > allocate all blocks underlying the page. This is not only unnecessary > > > but this way blocks can get leaked (e.g. if a page beyond EOF is marked > > > dirty but in the end write fails and i_size is not extended). > > > > Looks good: > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Matthew, Andrew, can one of you please pick up this fix? Thanks! I don't have a pull request pending for next merge window, so probably best if Andrew picks it up.
On Wed, 25 Jan 2023 15:23:51 +0100 Jan Kara <jack@suse.cz> wrote: > On Sun 08-01-23 09:25:10, Christoph Hellwig wrote: > > On Tue, Jan 03, 2023 at 11:44:30AM +0100, Jan Kara wrote: > > > When __mpage_writepage() is called for a page beyond EOF, it will go and > > > allocate all blocks underlying the page. This is not only unnecessary > > > but this way blocks can get leaked (e.g. if a page beyond EOF is marked > > > dirty but in the end write fails and i_size is not extended). > > > > Looks good: > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Matthew, Andrew, can one of you please pick up this fix? Thanks! > This was added to mm-stable (and hence linux-next) on Jan 18, as 4b89a37d54a0b.
On Wed 25-01-23 16:52:21, Andrew Morton wrote: > On Wed, 25 Jan 2023 15:23:51 +0100 Jan Kara <jack@suse.cz> wrote: > > > On Sun 08-01-23 09:25:10, Christoph Hellwig wrote: > > > On Tue, Jan 03, 2023 at 11:44:30AM +0100, Jan Kara wrote: > > > > When __mpage_writepage() is called for a page beyond EOF, it will go and > > > > allocate all blocks underlying the page. This is not only unnecessary > > > > but this way blocks can get leaked (e.g. if a page beyond EOF is marked > > > > dirty but in the end write fails and i_size is not extended). > > > > > > Looks good: > > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > Matthew, Andrew, can one of you please pick up this fix? Thanks! > > > > This was added to mm-stable (and hence linux-next) on Jan 18, as > 4b89a37d54a0b. Bah, thanks for reminder. I didn't see any reply in the thread and somehow my inbox searching from your commit-bot email failed. Sorry for the noise. Honza
diff --git a/fs/mpage.c b/fs/mpage.c index 0f8ae954a579..9f040c1d5912 100644 --- a/fs/mpage.c +++ b/fs/mpage.c @@ -524,6 +524,12 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc, */ BUG_ON(!PageUptodate(page)); block_in_file = (sector_t)page->index << (PAGE_SHIFT - blkbits); + /* + * Whole page beyond EOF? Skip allocating blocks to avoid leaking + * space. + */ + if (block_in_file >= (i_size + (1 << blkbits) - 1) >> blkbits) + goto page_is_mapped; last_block = (i_size - 1) >> blkbits; map_bh.b_page = page; for (page_block = 0; page_block < blocks_per_page; ) {
When __mpage_writepage() is called for a page beyond EOF, it will go and allocate all blocks underlying the page. This is not only unnecessary but this way blocks can get leaked (e.g. if a page beyond EOF is marked dirty but in the end write fails and i_size is not extended). Signed-off-by: Jan Kara <jack@suse.cz> --- fs/mpage.c | 6 ++++++ 1 file changed, 6 insertions(+)