Linux-mtd Archive on lore.kernel.org
 help / color / Atom feed
From: Minchan Kim <minchan@kernel.org>
To: Shakeel Butt <shakeelb@google.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	linux-raid@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>,
	Richard Weinberger <richard@nod.at>,
	LKML <linux-kernel@vger.kernel.org>,
	"open list:BLOCK LAYER" <linux-block@vger.kernel.org>,
	Song Liu <song@kernel.org>,
	dm-devel@redhat.com, linux-mtd@lists.infradead.org,
	Linux MM <linux-mm@kvack.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Cgroups <cgroups@vger.kernel.org>, Christoph Hellwig <hch@lst.de>,
	drbd-dev@lists.linbit.com
Subject: Re: [PATCH 11/14] mm: use SWP_SYNCHRONOUS_IO more intelligently
Date: Mon, 20 Jul 2020 22:46:17 -0700
Message-ID: <20200721054617.GA1879427@google.com> (raw)
In-Reply-To: <CALvZod7ACBnNX5W-gtTzheh8R-rxv1nB-5q7UcDUZ7BvtpakpA@mail.gmail.com>

Thanks for Ccing me, Shakeel.

On Mon, Jul 20, 2020 at 10:52:55AM -0700, Shakeel Butt wrote:
> +Minchan Kim
> 
> On Mon, Jul 20, 2020 at 12:52 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > There is no point in trying to call bdev_read_page if SWP_SYNCHRONOUS_IO
> > is not set, as the device won't support it.  Also there is no point in
> > trying a bio submission if bdev_read_page failed.
> 
> This will at least break the failure path of zram_rw_page().

Yes, it needs post processing for error propagaion like *page* handling
part in end_swap_bio_read(mostly, PG_error and PG_uptodate with pr_alert).
bdev_read_page's sematic doesn't need to be synchronous so it could just
submit the IO request and complete the IO afterward. In that case, we
need right error handling, too if the IO encoutered error. BIO fallback
makes it simple.

 * bdev_read_page() - Start reading a page from a block device
 * @bdev: The device to read the page from
 * @sector: The offset on the device to read the page to (need not be aligned)
 * @page: The page to read
 *
 * On entry, the page should be locked.  It will be unlocked when the page
 * has been read.  If the block driver implements rw_page synchronously,
 * that will be true on exit from this function, but it need not be.
 *
 * Errors returned by this function are usually "soft", eg out of memory, or
 * queue full; callers should try a different route to read this page rather
 * than propagate an error back up the stack.

The other concern about this patch is zram have used rw_page for a long
time even though sometime it doesn't declare BDI_CAP_SYNCHRONOUS_IO by itself
because rw_page shows 4~5% bandwidth improvement compared to bio-based.
The performance gain becomes more important these day because compressor
becomes more fast day by day.

> 
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  mm/page_io.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/page_io.c b/mm/page_io.c
> > index ccda7679008851..63b44b8221af0f 100644
> > --- a/mm/page_io.c
> > +++ b/mm/page_io.c
> > @@ -403,8 +403,11 @@ int swap_readpage(struct page *page, bool synchronous)
> >                 goto out;
> >         }
> >
> > -       ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
> > -       if (!ret) {
> > +       if (sis->flags & SWP_SYNCHRONOUS_IO) {
> > +               ret = bdev_read_page(sis->bdev, swap_page_sector(page), page);
> > +               if (ret)
> > +                       goto out;
> > +
> >                 if (trylock_page(page)) {
> >                         swap_slot_free_notify(page);
> >                         unlock_page(page);
> > --
> > 2.27.0
> >

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-20  7:51 bdi cleanups Christoph Hellwig
2020-07-20  7:51 ` [PATCH 01/14] fs: remove the unused SB_I_MULTIROOT flag Christoph Hellwig
2020-07-20  7:51 ` [PATCH 02/14] drbd: remove dead code in device_to_statistics Christoph Hellwig
2020-07-20  7:51 ` [PATCH 03/14] drbd: remove RB_CONGESTED_REMOTE Christoph Hellwig
2020-07-20  7:51 ` [PATCH 04/14] bdi: initialize ->ra_pages in bdi_init Christoph Hellwig
2020-07-20 11:58   ` Richard Weinberger
2020-07-20 12:07     ` Christoph Hellwig
2020-07-20 12:37       ` Artem Bityutskiy
2020-07-20 21:27         ` Richard Weinberger
2020-07-21  5:00           ` Christoph Hellwig
2020-07-20  7:51 ` [PATCH 05/14] md: update the optimal I/O size on reshape Christoph Hellwig
2020-07-20  7:51 ` [PATCH 06/14] block: lift setting the readahead size into the block layer Christoph Hellwig
2020-07-20  7:51 ` [PATCH 07/14] block: make QUEUE_SYSFS_BIT_FNS a little more useful Christoph Hellwig
2020-07-20  7:51 ` [PATCH 08/14] block: add helper macros for queue sysfs entries Christoph Hellwig
2020-07-20  7:51 ` [PATCH 09/14] bdi: remove BDI_CAP_CGROUP_WRITEBACK Christoph Hellwig
2020-07-20  7:51 ` [PATCH 10/14] bdi: remove BDI_CAP_SYNCHRONOUS_IO Christoph Hellwig
2020-07-20  7:51 ` [PATCH 11/14] mm: use SWP_SYNCHRONOUS_IO more intelligently Christoph Hellwig
2020-07-20 17:52   ` Shakeel Butt
2020-07-21  5:46     ` Minchan Kim [this message]
2020-07-20  7:51 ` [PATCH 12/14] bdi: replace BDI_CAP_STABLE_WRITES with a queue and a sb flag Christoph Hellwig
2020-07-20  7:51 ` [PATCH 13/14] bdi: invert BDI_CAP_NO_ACCT_WB Christoph Hellwig
2020-07-20  7:51 ` [PATCH 14/14] bdi: replace BDI_CAP_NO_{WRITEBACK, ACCT_DIRTY} with a single flag Christoph Hellwig
2020-07-22  6:25 bdi cleanups v2 Christoph Hellwig
2020-07-22  6:25 ` [PATCH 11/14] mm: use SWP_SYNCHRONOUS_IO more intelligently Christoph Hellwig
2020-07-22 16:26   ` Shakeel Butt
2020-07-24  7:32 bdi cleanups v3 Christoph Hellwig
2020-07-24  7:33 ` [PATCH 11/14] mm: use SWP_SYNCHRONOUS_IO more intelligently Christoph Hellwig
2020-07-28 12:28   ` Johannes Thumshirn
2020-07-26 15:03 bdi cleanups v3 Christoph Hellwig
2020-07-26 15:03 ` [PATCH 11/14] mm: use SWP_SYNCHRONOUS_IO more intelligently Christoph Hellwig

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200721054617.GA1879427@google.com \
    --to=minchan@kernel.org \
    --cc=axboe@kernel.dk \
    --cc=cgroups@vger.kernel.org \
    --cc=dm-devel@redhat.com \
    --cc=drbd-dev@lists.linbit.com \
    --cc=hch@lst.de \
    --cc=hdegoede@redhat.com \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-raid@vger.kernel.org \
    --cc=richard@nod.at \
    --cc=shakeelb@google.com \
    --cc=song@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mtd Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mtd/0 linux-mtd/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mtd linux-mtd/ https://lore.kernel.org/linux-mtd \
		linux-mtd@lists.infradead.org
	public-inbox-index linux-mtd

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mtd


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git