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: [RFC 2/2] ext4: Move ext4_fiemap to iomap infrastructure
To:     Jan Kara <jack@suse.cz>
Cc:     linux-ext4@vger.kernel.org, adilger.kernel@dilger.ca,
        tytso@mit.edu, mbobrowski@mbobrowski.org,
        linux-fsdevel@vger.kernel.org
References: <20190820130634.25954-1-riteshh@linux.ibm.com>
 <20190820130634.25954-3-riteshh@linux.ibm.com>
 <20191016084611.GB30337@quack2.suse.cz>
From:   Ritesh Harjani <riteshh@linux.ibm.com>
Date:   Wed, 16 Oct 2019 18:28:32 +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: <20191016084611.GB30337@quack2.suse.cz>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <20191016125833.79334A405B@b06wcsmtp001.portsmouth.uk.ibm.com>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk


On 10/16/19 2:16 PM, Jan Kara wrote:
> On Tue 20-08-19 18:36:34, Ritesh Harjani wrote:
>> This moves ext4_fiemap to use iomap infrastructure.
>>
>> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com>
> 
> Thanks for the patch. I like how it removes lots of code :) The patch looks
> good to me, just two small comments below. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Sure, thanks. It's been some quite some time.
Let me rebase these patches on latest upstream kernel.

> 
>> +static int ext4_xattr_iomap_fiemap(struct inode *inode, struct iomap *iomap)
>>   {
>> -	__u64 physical = 0;
>> -	__u64 length;
>> -	__u32 flags = FIEMAP_EXTENT_LAST;
>> +	u64 physical = 0;
>> +	u64 length;
>>   	int blockbits = inode->i_sb->s_blocksize_bits;
>> -	int error = 0;
>> +	int ret = 0;
>>   
>>   	/* in-inode? */
>>   	if (ext4_test_inode_state(inode, EXT4_STATE_XATTR)) {
>>   		struct ext4_iloc iloc;
>> -		int offset;	/* offset of xattr in inode */
>> +		int offset;     /* offset of xattr in inode */
>>   
>> -		error = ext4_get_inode_loc(inode, &iloc);
>> -		if (error)
>> -			return error;
>> +		ret = ext4_get_inode_loc(inode, &iloc);
>> +		if (ret)
>> +			goto out;
>>   		physical = (__u64)iloc.bh->b_blocknr << blockbits;
>>   		offset = EXT4_GOOD_OLD_INODE_SIZE +
>>   				EXT4_I(inode)->i_extra_isize;
> 
> Hum, I see you've just copied this from the old code but this actually
> won't give full information for FIEMAP of inode with extended attribute
> inodes. 

Could you please elaborate on above? I am anyway taking a look at it in
parallel. I can provide that as a separate patch, if required.


> Not something you need to fix for your patch but I wanted to
> mention this so that it doesn't get lost.

Sure :)

> 
>>   		physical += offset;
>>   		length = EXT4_SB(inode->i_sb)->s_inode_size - offset;
>> -		flags |= FIEMAP_EXTENT_DATA_INLINE;
>>   		brelse(iloc.bh);
>>   	} else { /* external block */
>>   		physical = (__u64)EXT4_I(inode)->i_file_acl << blockbits;
>>   		length = inode->i_sb->s_blocksize;
>>   	}
>>   
>> -	if (physical)
>> -		error = fiemap_fill_next_extent(fieinfo, 0, physical,
>> -						length, flags);
>> -	return (error < 0 ? error : 0);
>> +	iomap->addr = physical;
>> +	iomap->offset = 0;
>> +	iomap->length = length;
>> +	iomap->type = IOMAP_INLINE;
>> +	iomap->flags = 0;
>> +out:
>> +	return ret;
>>   }
> 
> ...
> 
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index d6a34214e9df..92705e99e07c 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -3581,15 +3581,24 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
>>   		iomap->type = delalloc ? IOMAP_DELALLOC : IOMAP_HOLE;
>>   		iomap->addr = IOMAP_NULL_ADDR;
>>   	} else {
>> -		if (map.m_flags & EXT4_MAP_MAPPED) {
>> -			iomap->type = IOMAP_MAPPED;
>> -		} else if (map.m_flags & EXT4_MAP_UNWRITTEN) {
>> +		/*
>> +		 * There can be a case where map.m_flags may
>> +		 * have both EXT4_MAP_UNWRITTEN & EXT4_MAP_MERGED
>> +		 * set. This is when we do fallocate first and
>> +		 * then try to write to that area, then it may have
>> +		 * both flags set. So check for unwritten first.
>> +		 */
>> +		if (map.m_flags & EXT4_MAP_UNWRITTEN) {
>>   			iomap->type = IOMAP_UNWRITTEN;
>> +		} else if (map.m_flags & EXT4_MAP_MAPPED) {
>> +			iomap->type = IOMAP_MAPPED;
> 
> This should be already part of Matthew's series so once you rebase on top
> of it, you can just drop this hunk.

Sure, will do.

-riteshh