Re: [PATCH 4/4] block: Optionally snapshot page contents to provide stable pages during write

["Darrick J. Wong" <darrick.wong@oracle.com>]

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@suse.cz>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html