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
