Re: [PATCH v2 2/6] ext4: move inode extension/truncate code out from ext4_iomap_end()

[Matthew Bobrowski <mbobrowski@mbobrowski.org>]

On Mon, Sep 09, 2019 at 01:47:28PM +0530, Ritesh Harjani wrote:
> On 9/9/19 4:49 AM, Matthew Bobrowski wrote:
> > +static int ext4_handle_inode_extension(struct inode *inode, loff_t offset,
> > +				       ssize_t len, size_t count)
> > +{
> > +	handle_t *handle;
> > +	bool truncate = false;
> > +	ext4_lblk_t written_blk, end_blk;
> > +	int ret = 0, blkbits = inode->i_blkbits;
> > +
> > +	handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
> > +	if (IS_ERR(handle)) {
> > +		ret = PTR_ERR(handle);
> > +		goto orphan_del;
> > +	}
> > +
> > +	if (ext4_update_inode_size(inode, offset + len))
> > +		ext4_mark_inode_dirty(handle, inode);
> > +
> > +	/*
> > +	 * We may need truncate allocated but not written blocks
> > +	 * beyond EOF.
> > +	 */
> > +	written_blk = ALIGN(offset + len, 1 << blkbits);
> > +	end_blk = ALIGN(offset + len + count, 1 << blkbits);
> 
> why add len in end_blk calculation?
> shouldn't this be like below?
> 	end_blk = ALIGN(offset + count, 1 << blkbits);

I don't believe that would be entirely correct. The reason being is that the
'end_blk' is meant to represent the last logical block which we should expect
to have used for the write operation. So, we have the 'offset' which
represents starting point, 'len' which is the amount of data that has been
written, and 'count' which is the amount of data that we still have left over
in the 'iter', if any.

The count in the 'iter' is decremented as that data is copied from it. So if
we did use 'offset' + 'count', in the instance of a short write, we
potentially wouldn't truncate any of the allocated but not written blocks. I
guess this would hold true for the DAX code path at this point, seeing as
though for the DIO case we pass in '0'.

> > +/*
> > + * The inode may have been placed onto the orphan list or has had
> > + * blocks allocated beyond EOF as a result of an extension. We need to
> > + * ensure that any necessary cleanup routines are performed if the
> > + * error path has been taken for a write.
> > + */
> > +static int ext4_handle_failed_inode_extension(struct inode *inode, loff_t size)
> > +{
> > +	int ret = 0;
> 
> No need of ret anyways.
> 
> 
> > +	handle_t *handle;
> > +
> > +	if (size > i_size_read(inode))
> > +		ext4_truncate_failed_write(inode);
> > +
> > +	if (!list_empty(&EXT4_I(inode)->i_orphan)) {
> > +		handle = ext4_journal_start(inode, EXT4_HT_INODE, 2);
> > +		if (IS_ERR(handle)) {
> > +			if (inode->i_nlink)
> > +				ext4_orphan_del(NULL, inode);
> > +			return PTR_ERR(handle);
> > +		}
> > +		if (inode->i_nlink)
> > +			ext4_orphan_del(handle, inode);
> > +		ext4_journal_stop(handle);
> > +	}
> > +	return ret;
> 
> can directly call for `return 0;`

True.

--<M>--