lkml.org 
[lkml]   [2022]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH] erofs/zmap.c: Bail out when no further region remains
From
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

\
 
 \ /
  Last update: 2022-12-09 09:30    [W:0.060 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site