linux-kernel - Re: [PATCH 1/2] iomap: support tail packing inline read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YPGQB3zT4Wp4Q38X@B-P7TQMD6M-0146.local>
Date:   Fri, 16 Jul 2021 21:56:23 +0800
From:   Gao Xiang <hsiangkao@...ux.alibaba.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-erofs@...ts.ozlabs.org, linux-fsdevel@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        "Darrick J. Wong" <djwong@...nel.org>,
        Christoph Hellwig <hch@...radead.org>,
        Chao Yu <chao@...nel.org>, Liu Bo <bo.liu@...ux.alibaba.com>,
        Joseph Qi <joseph.qi@...ux.alibaba.com>,
        Liu Jiang <gerry@...ux.alibaba.com>
Subject: Re: [PATCH 1/2] iomap: support tail packing inline read

Hi Matthew,

On Fri, Jul 16, 2021 at 02:02:29PM +0100, Matthew Wilcox wrote:
> On Fri, Jul 16, 2021 at 01:07:23PM +0800, Gao Xiang wrote:
> > This tries to add tail packing inline read to iomap. Different from
> > the previous approach, it only marks the block range uptodate in the
> > page it covers.
> 
> Why?  This path is called under two circumstances: readahead and readpage.
> In both cases, we're trying to bring the entire page uptodate.  The inline
> extent is always the tail of the file, so we may as well zero the part of
> the page past the end of file and mark the entire page uptodate instead
> and leaving the end of the page !uptodate.
> 
> I see the case where, eg, we have the first 2048 bytes of the file
> out-of-inode and then 20 bytes in the inode.  So we'll create the iop
> for the head of the file, but then we may as well finish the entire
> PAGE_SIZE chunk as part of this iteration rather than update 2048-3071
> as being uptodate and leave the 3072-4095 block for a future iteration.

Thanks for your comments. Hmm... If I understand the words above correctly,
what I'd like to do is to cover the inline extents (blocks) only
reported by iomap_begin() rather than handling other (maybe)
logical-not-strictly-relevant areas such as post-EOF (even pages
will be finally entirely uptodated), I think such zeroed area should
be handled by from the point of view of the extent itself

         if (iomap_block_needs_zeroing(inode, iomap, pos)) {
                 zero_user(page, poff, plen);
                 iomap_set_range_uptodate(page, poff, plen);
                 goto done;
         }

The benefits I can think out are 1) it makes the logic understand
easier and no special cases just for tail-packing handling 2) it can
be then used for any inline extent cases (I mean e.g. in the middle of
the file) rather than just tail-packing inline blocks although currently
there is a BUG_ON to prevent this but it's easier to extend even further.
3) it can be used as a part for later partial page uptodate logic in
order to match the legacy buffer_head logic (I remember something if my
memory is not broken about this...)

Thanks,
Gao Xiang