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

[Ritesh Harjani <riteshh@linux.ibm.com>]

On 9/10/19 3:56 PM, Matthew Bobrowski wrote:
> 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.
> 

Agree. Yes, I see that you are passing iov_iter_count(from) as a param,
after the dax write.


> 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'.

Agreed.


> 
>>> +/*
>>> + * 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>--
>