On Mon, Dec 17, 2012 at 11:23:59AM +0100, Jan Kara wrote:
> On Thu 13-12-12 00:08:11, Darrick J. Wong wrote:
> > Several complaints have been received regarding long file write latencies when
> > memory pages must be held stable during writeback. Since it might not be
> > acceptable to stall programs for the entire duration of a page write (which may
> > take many milliseconds even on good hardware), enable a second strategy wherein
> > pages are snapshotted as part of submit_bio; the snapshot can be held stable
> > while writes continue.
> >
> > This provides a band-aid to provide stable page writes on jbd without needing
> > to backport the fixed locking scheme in jbd2. A mount option is added to ext4
> > to allow administrators to enable it there.
> >
> > For those wondering about the ext3 bandage -- fixing the jbd locking (which was
> > done as part of ext4dev years ago) is a lot of surgery, and setting
> > PG_writeback on data pages when we actually hold the page lock dropped ext3
> > performance by nearly an order of magnitude. If we're going to migrate iscsi
> > and raid to use stable page writes, the complaints about high latency will
> > likely return. We might as well centralize their page snapshotting thing to
> > one place.
> Umm, I kind of like this solution for ext3...
:)
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index a3f8ddd..78db0e1 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -224,6 +224,16 @@ config BOUNCE
> > def_bool y
> > depends on BLOCK && MMU && (ZONE_DMA || HIGHMEM)
> >
> > +# On the 'tile' arch, USB OHCI needs the bounce pool since tilegx will often
> > +# have more than 4GB of memory, but we don't currently use the IOTLB to present
> > +# a 32-bit address to OHCI. So we need to use a bounce pool instead.
> > +#
> > +# We also use the bounce pool to provide stable page writes for users that
> > +# don't (or can't) afford the wait latency.
> > +config NEED_BOUNCE_POOL
> > + bool
> > + default y if (TILE && USB_OHCI_HCD) || (BLK_DEV_INTEGRITY && (EXT3_FS || EXT4_FS))
> > +
> This means that NEED_BOUNCE_POOL is going to be enabled on pretty much
> any distro kernel...
I want to drop EXT4_FS from that default line. Then we only need it in the
case where EXT4_USE_FOR_EXT23=y hasn't taken the place of EXT3_FS=y.
<shrug> Has anyone actually done that?
Heh, even *I* haven't done that.
> > config NR_QUICK
> > int
> > depends on QUICKLIST
> > diff --git a/mm/bounce.c b/mm/bounce.c
> > index 0420867..fa11935 100644
> > --- a/mm/bounce.c
> > +++ b/mm/bounce.c
> > @@ -178,6 +178,38 @@ static void bounce_end_io_read_isa(struct bio *bio, int err)
> > __bounce_end_io_read(bio, isa_page_pool, err);
> > }
> >
> > +#ifdef CONFIG_NEED_BOUNCE_POOL
> > +static int might_snapshot_stable_page_write(struct bio **bio_orig)
> > +{
> > + return bio_data_dir(*bio_orig) == WRITE;
> > +}
> ... so might might_snapshot_stable_page_write() will be true for each
> write. And thus blk_queue_bounce() becomes considerably more expensive?
> Also calling should_snapshot_stable_pages() for every page seems to be
> stupid since its result is going to be the same for all the pages in the
> bio (well, I could imagine setups where it won't be but do we want to
> support them?).
>
> So cannot we just make a function like should_snapshot_stable_pages() to
> test whether we really need the bouncing, use it in blk_queue_bounce() and
> then pass the information to __blk_queue_bounce() if needed?
Yes. I'd actually considered simply calling should_snapshot() on the first
bio_vec page and passing that information through, but thought I should play it
safe for the first revision.
However, a bio targets a single block device, so this seems like a safe
optimization.
--D
>
> Honza
>
> > +static int should_snapshot_stable_pages(struct page *page, int rw)
> > +{
> > + struct backing_dev_info *bdi;
> > + struct address_space *mapping = page_mapping(page);
> > +
> > + if (!mapping)
> > + return 0;
> > + bdi = mapping->backing_dev_info;
> > + if (!bdi_cap_stable_pages_required(bdi))
> > + return 0;
> > +
> > + return mapping->host->i_sb->s_flags & MS_SNAP_STABLE &&
> > + rw == WRITE;
> > +}
> > +#else
> > +static int might_snapshot_stable_page_write(struct bio **bio_orig)
> > +{
> > + return 0;
> > +}
> > +
> > +static int should_snapshot_static_pages(struct page *page, int rw)
> > +{
> > + return 0;
> > +}
> > +#endif /* CONFIG_NEED_BOUNCE_POOL */
> > +
> > static void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig,
> > mempool_t *pool)
> > {
> > @@ -192,7 +224,8 @@ static void __blk_queue_bounce(struct request_queue *q, struct bio **bio_orig,
> > /*
> > * is destination page below bounce pfn?
> > */
> > - if (page_to_pfn(page) <= queue_bounce_pfn(q))
> > + if (page_to_pfn(page) <= queue_bounce_pfn(q) &&
> > + !should_snapshot_stable_pages(page, rw))
> > continue;
> >
> > /*
> > @@ -284,7 +317,8 @@ void blk_queue_bounce(struct request_queue *q, struct bio **bio_orig)
> > * don't waste time iterating over bio segments
> > */
> > if (!(q->bounce_gfp & GFP_DMA)) {
> > - if (queue_bounce_pfn(q) >= blk_max_pfn)
> > + if (queue_bounce_pfn(q) >= blk_max_pfn &&
> > + !might_snapshot_stable_page_write(bio_orig))
> > return;
> > pool = page_pool;
> > } else {
> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> > index 3e4a8cc..fbd8efb 100644
> > --- a/mm/page-writeback.c
> > +++ b/mm/page-writeback.c
> > @@ -2291,6 +2291,10 @@ void wait_for_stable_page(struct page *page)
> >
> > if (!bdi_cap_stable_pages_required(bdi))
> > return;
> > +#ifdef CONFIG_NEED_BOUNCE_POOL
> > + if (mapping->host->i_sb->s_flags & MS_SNAP_STABLE)
> > + return;
> > +#endif /* CONFIG_NEED_BOUNCE_POOL */
> >
> > wait_on_page_writeback(page);
> > }
> >
> --
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
