linux-kernel - Re: [RFC PATCH 1/3] filemap: convert page_endio to folio

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: <d6cde35e-359a-e837-d2e0-f2bd362f2c3e@samsung.com>
Date:   Thu, 16 Mar 2023 11:04:54 +0100
From:   Pankaj Raghav <p.raghav@...sung.com>
To:     Christoph Hellwig <hch@...radead.org>
CC:     <hubcap@...ibond.com>, <senozhatsky@...omium.org>,
        <martin@...ibond.com>, <willy@...radead.org>, <minchan@...nel.org>,
        <viro@...iv.linux.org.uk>, <brauner@...nel.org>, <axboe@...nel.dk>,
        <akpm@...ux-foundation.org>, <linux-kernel@...r.kernel.org>,
        <linux-block@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
        <linux-mm@...ck.org>, <gost.dev@...sung.com>, <mcgrof@...nel.org>,
        <devel@...ts.orangefs.org>
Subject: Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

Hi Christoph,

On 2023-03-15 15:56, Christoph Hellwig wrote:
> Can we take a step back and figure out if page_endio is a good
> idea to start with?
> 
> The zram usage seems clearly wrong to me.  zram is a block driver
> and does not own the pages, so it shouldn't touch any of the page
> state.  It seems like this mostly operates on it's own
> pages allocated using alloc_page so the harm might not be horrible
> at least.
> 
It looks like this endio function is called when alloc_page is used (for
partial IO) to trigger writeback from the user space `echo "idle" >
/sys/block/zram0/writeback`.

I don't understand when you say the harm might not be horrible if we don't
call folio_endio here. Do you think it is just safe to remove the call to
folio_endio function?

> orangefs uses it on readahead pages, with ret known for the whole
> iteration.  So one quick loop for the success and one for the
> failure case would look simpler an more obvious.
> 
Got it. Something like this?

@@ -266,18 +266,23 @@ static void orangefs_readahead(struct
readahead_control *rac)
        iov_iter_xarray(&iter, ITER_DEST, i_pages, offset,
readahead_length(rac));

        /* read in the pages. */
-       if ((ret = wait_for_direct_io(ORANGEFS_IO_READ, inode,
-                       &offset, &iter, readahead_length(rac),
-                       inode->i_size, NULL, NULL, rac->file)) < 0)
+       if ((ret = wait_for_direct_io(ORANGEFS_IO_READ, inode, &offset, &iter,
+                                     readahead_length(rac), inode->i_size,
+                                     NULL, NULL, rac->file)) < 0) {
                gossip_debug(GOSSIP_FILE_DEBUG,
-                       "%s: wait_for_direct_io failed. \n", __func__);
-       else
-               ret = 0;
+                            "%s: wait_for_direct_io failed. \n", __func__);

-       /* clean up. */
-       while ((page = readahead_page(rac))) {
-               page_endio(page, false, ret);
-               put_page(page);
+               while ((folio = readahead_folio(rac))) {
+                       folio_clear_uptodate(folio);
+                       folio_set_error(folio);
+                       folio_unlock(folio);
+               }
+               return;
+       }
+
+       while ((folio = readahead_folio(rac))) {
+               folio_mark_uptodate(folio);
+               folio_unlock(folio);
        }
 }

> mpage really should use separate end_io handler for read vs write
> as well like most other aops do.
> 

I don't know if this is the right abstraction to do the switch, but let me
know what you think:
@@ -59,7 +59,8 @@ static void mpage_end_io(struct bio *bio)
static struct bio *mpage_bio_submit(struct bio *bio)
 {
-       bio->bi_end_io = mpage_end_io;
+       bio->bi_end_io = (op_is_write(bio_op(bio))) ? mpage_write_end_io :
+                                                     mpage_read_end_io;
        guard_bio_eod(bio);
        submit_bio(bio);
        return NULL;
And mpage_{write,read}_end_io will iterate over the folio and call the
respective functions.

> So overall I'd be happier to just kill the helper.