Re: [PATCH 5/8] xfs: CIL checkpoint flushes caches unconditionally

From: Christoph Hellwig <hch@infradead.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/8] xfs: CIL checkpoint flushes caches unconditionally
Date: Thu, 25 Feb 2021 09:42:02 +0100	[thread overview]
Message-ID: <YDdi2pgRDJtv5M8P@infradead.org> (raw)
In-Reply-To: <20210223033442.3267258-6-david@fromorbit.com>

This looks ok, but please make add two trivial checks that the device
actually supports/needs flushes.  All that magic of allocating a bio

On Tue, Feb 23, 2021 at 02:34:39PM +1100, Dave Chinner wrote:
>  	new_ctx = kmem_zalloc(sizeof(*new_ctx), KM_NOFS);
>  	new_ctx->ticket = xlog_cil_ticket_alloc(log);
> @@ -719,10 +720,24 @@ xlog_cil_push_work(
>  	spin_unlock(&cil->xc_push_lock);
>  
>  	/*
> -	 * pull all the log vectors off the items in the CIL, and
> -	 * remove the items from the CIL. We don't need the CIL lock
> -	 * here because it's only needed on the transaction commit
> -	 * side which is currently locked out by the flush lock.
> +	 * The CIL is stable at this point - nothing new will be added to it
> +	 * because we hold the flush lock exclusively. Hence we can now issue
> +	 * a cache flush to ensure all the completed metadata in the journal we
> +	 * are about to overwrite is on stable storage.
> +	 *
> +	 * This avoids the need to have the iclogs issue REQ_PREFLUSH based
> +	 * cache flushes to provide this ordering guarantee, and hence for CIL
> +	 * checkpoints that require hundreds or thousands of log writes no
> +	 * longer need to issue device cache flushes to provide metadata
> +	 * writeback ordering.
> +	 */
> +	xfs_flush_bdev_async(log->l_mp->m_ddev_targp->bt_bdev, &bdev_flush);

This still causes a bio allocation, also even if the device does not need
flush.  Please also use bio_init on a bio passed into xfs_flush_bdev_async to
avoid that, and make the whole code conditional to only run if we actually need
to flush caches.