[PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[Jan Kara]

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;
 	map_bh.b_page = page;
 	for (page_block = 0; page_block < blocks_per_page; ) {
-- 
2.35.3

Re: [PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[Al Viro]

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?

Re: [PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[Jan Kara]

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

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[Christoph Hellwig]

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>

Re: [PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[Matthew Wilcox]

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.

Re: [PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[Andrew Morton]

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.

Re: [PATCH] fs: don't allocate blocks beyond EOF from __mpage_writepage

[Jan Kara]

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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR