Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7c836cf0-d978-c892-bec8-0992e2347512@siddh.me>
Date:   Fri, 9 Dec 2022 13:59:02 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [RFC PATCH] erofs/zmap.c: Bail out when no further region remains
Content-Language: en-US, en-GB, hi-IN
To:     linux-kernel <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Yue Hu <huyue2@coolpad.com>,
        linux-erofs <linux-erofs@lists.ozlabs.org>
References: <Y3MGf3TzgKpAz4IP@B-P7TQMD6M-0146.local>
 <917344b4-4256-6d77-b89b-07fa96ec4539@siddh.me>
 <Y3Nu+TNRp6Fv3L19@B-P7TQMD6M-0146.local>
 <Y5K+p6td52QppRZt@B-P7TQMD6M-0146.local>
From:   Siddh Raman Pant <code@siddh.me>
In-Reply-To: <Y5K+p6td52QppRZt@B-P7TQMD6M-0146.local>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Fri, Dec 09 2022 at 10:20:47 +0530, Gao Xiang wrote:
> Hi Siddh,
> 
> On Tue, Nov 15, 2022 at 06:50:33PM +0800, Gao Xiang wrote:
>> On Tue, Nov 15, 2022 at 03:39:38PM +0530, Siddh Raman Pant via Linux-erofs wrote:
>>> On Tue, 15 Nov 2022 08:54:47 +0530, Gao Xiang wrote:
>>>> I just wonder if we should return -EINVAL for post-EOF cases or
>>>> IOMAP_HOLE with arbitrary length?
>>>
>>> Since it has been observed that length can be zeroed, and we
>>> must stop, I think we should return an error appropriately.
>>>
>>> For a read-only filesystem, we probably don't really need to
>>> care what's after the EOF or in unmapped regions, nothing can
>>> be changed/extended. The definition of IOMAP_HOLE in iomap.h
>>> says it stands for "no blocks allocated, need allocation".
>>
>> For fiemap implementation, yes.  So it looks fine to me.
>>
>> Let's see what other people think.  Anyway, I'd like to apply it later.
>>
> 
> Very sorry for late response.
> 
> I've just confirmed that the reason is that
> 
> 796                 /*
> 797                  * No strict rule how to describe extents for post EOF, yet
> 798                  * we need do like below. Otherwise, iomap itself will get
> 799                  * into an endless loop on post EOF.
> 800                  */
> 801                 if (iomap->offset >= inode->i_size)
> 802                         iomap->length = length + map.m_la - offset;
> 
> Here iomap->length should be length + offset - map.m_la here. Because
> the extent start (map.m_la) is always no more than requested `offset'.
> 
> We should need this code sub-block since userspace (filefrag -v) could
> pass
> ioctl(3, FS_IOC_FIEMAP, {fm_start=0, fm_length=18446744073709551615, fm_flags=0, fm_extent_count=292} => {fm_flags=0, fm_mapped_extents=68, ...}) = 0
> 
> without this sub-block, fiemap could get into a very long loop as below:
> [  574.030856][ T7030] erofs: m_la 70000000 offset 70457397 length 9223372036784318410 m_llen 457398
> [  574.031622][ T7030] erofs: m_la 70000000 offset 70457398 length 9223372036784318409 m_llen 457399
> [  574.032397][ T7030] erofs: m_la 70000000 offset 70457399 length 9223372036784318408 m_llen 457400

Thanks for the detailed explanation!

> So could you fix this as?
> 	iomap->length = length + offset - map.m_la;
> 
> I've already verified it can properly resolve the issue and do the
> correct thing although I'd like to submit this later since we're quite
> close to the merge window.
> 
> Thanks,
> Gao Xiang

Sure, I'll send the patch for now, which can be merged after the window.

Thanks,
Siddh