Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 2/6] ext4: move inode extension/truncate code out from
 ext4_iomap_end()
To:     Matthew Bobrowski <mbobrowski@mbobrowski.org>
Cc:     tytso@mit.edu, jack@suse.cz, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        david@fromorbit.com, hch@infradead.org, darrick.wong@oracle.com
References: <cover.1567978633.git.mbobrowski@mbobrowski.org>
 <c1e9b23ced988587dfec399021a5b62983745842.1567978633.git.mbobrowski@mbobrowski.org>
 <20190909081729.3555C42041@d06av24.portsmouth.uk.ibm.com>
 <20190910102643.GA9013@bobrowski>
From:   Ritesh Harjani <riteshh@linux.ibm.com>
Date:   Tue, 10 Sep 2019 16:46:30 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <20190910102643.GA9013@bobrowski>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <20190910111631.08C5A4C052@d06av22.portsmouth.uk.ibm.com>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk


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