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.
