Re: [PATCH v7 4/9] block: Add core atomic write support

[Hannes Reinecke <hare@suse.de>]

On 6/2/24 16:09, John Garry wrote:
> Add atomic write support, as follows:
> - add helper functions to get request_queue atomic write limits
> - report request_queue atomic write support limits to sysfs and update Doc
> - support to safely merge atomic writes
> - deal with splitting atomic writes
> - misc helper functions
> - add a per-request atomic write flag
> 
> New request_queue limits are added, as follows:
> - atomic_write_hw_max is set by the block driver and is the maximum length
>    of an atomic write which the device may support. It is not
>    necessarily a power-of-2.
> - atomic_write_max_sectors is derived from atomic_write_hw_max_sectors and
>    max_hw_sectors. It is always a power-of-2. Atomic writes may be merged,
>    and atomic_write_max_sectors would be the limit on a merged atomic write
>    request size. This value is not capped at max_sectors, as the value in
>    max_sectors can be controlled from userspace, and it would only cause
>    trouble if userspace could limit atomic_write_unit_max_bytes and the
>    other atomic write limits.
> - atomic_write_hw_unit_{min,max} are set by the block driver and are the
>    min/max length of an atomic write unit which the device may support. They
>    both must be a power-of-2. Typically atomic_write_hw_unit_max will hold
>    the same value as atomic_write_hw_max.
> - atomic_write_unit_{min,max} are derived from
>    atomic_write_hw_unit_{min,max}, max_hw_sectors, and block core limits.
>    Both min and max values must be a power-of-2.
> - atomic_write_hw_boundary is set by the block driver. If non-zero, it
>    indicates an LBA space boundary at which an atomic write straddles no
>    longer is atomically executed by the disk. The value must be a
>    power-of-2. Note that it would be acceptable to enforce a rule that
>    atomic_write_hw_boundary_sectors is a multiple of
>    atomic_write_hw_unit_max, but the resultant code would be more
>    complicated.
> 
> All atomic writes limits are by default set 0 to indicate no atomic write
> support. Even though it is assumed by Linux that a logical block can always
> be atomically written, we ignore this as it is not of particular interest.
> Stacked devices are just not supported either for now.
> 
> An atomic write must always be submitted to the block driver as part of a
> single request. As such, only a single BIO must be submitted to the block
> layer for an atomic write. When a single atomic write BIO is submitted, it
> cannot be split. As such, atomic_write_unit_{max, min}_bytes are limited
> by the maximum guaranteed BIO size which will not be required to be split.
> This max size is calculated by request_queue max segments and the number
> of bvecs a BIO can fit, BIO_MAX_VECS. Currently we rely on userspace
> issuing a write with iovcnt=1 for pwritev2() - as such, we can rely on each
> segment containing PAGE_SIZE of data, apart from the first+last, which each
> can fit logical block size of data. The first+last will be LBS
> length/aligned as we rely on direct IO alignment rules also.
> 
> New sysfs files are added to report the following atomic write limits:
> - atomic_write_unit_max_bytes - same as atomic_write_unit_max_sectors in
> 				bytes
> - atomic_write_unit_min_bytes - same as atomic_write_unit_min_sectors in
> 				bytes
> - atomic_write_boundary_bytes - same as atomic_write_hw_boundary_sectors in
> 				bytes
> - atomic_write_max_bytes      - same as atomic_write_max_sectors in bytes
> 
> Atomic writes may only be merged with other atomic writes and only under
> the following conditions:
> - total resultant request length <= atomic_write_max_bytes
> - the merged write does not straddle a boundary
> 
> Helper function bdev_can_atomic_write() is added to indicate whether
> atomic writes may be issued to a bdev. If a bdev is a partition, the
> partition start must be aligned with both atomic_write_unit_min_sectors
> and atomic_write_hw_boundary_sectors.
> 
> FSes will rely on the block layer to validate that an atomic write BIO
> submitted will be of valid size, so add blk_validate_atomic_write_op_size()
> for this purpose. Userspace expects an atomic write which is of invalid
> size to be rejected with -EINVAL, so add BLK_STS_INVAL for this. Also use
> BLK_STS_INVAL for when a BIO needs to be split, as this should mean an
> invalid size BIO.
> 
> Flag REQ_ATOMIC is used for indicating an atomic write.
> 
> Co-developed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: Himanshu Madhani <himanshu.madhani@oracle.com>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>   Documentation/ABI/stable/sysfs-block | 53 ++++++++++++++++
>   block/blk-core.c                     | 19 ++++++
>   block/blk-merge.c                    | 95 +++++++++++++++++++++++++++-
>   block/blk-settings.c                 | 52 +++++++++++++++
>   block/blk-sysfs.c                    | 33 ++++++++++
>   block/blk.h                          |  3 +
>   include/linux/blk_types.h            |  8 ++-
>   include/linux/blkdev.h               | 54 ++++++++++++++++
>   8 files changed, 315 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/stable/sysfs-block b/Documentation/ABI/stable/sysfs-block
> index 831f19a32e08..cea8856f798d 100644
> --- a/Documentation/ABI/stable/sysfs-block
> +++ b/Documentation/ABI/stable/sysfs-block
> @@ -21,6 +21,59 @@ Description:
>   		device is offset from the internal allocation unit's
>   		natural alignment.
>   
> +What:		/sys/block/<disk>/atomic_write_max_bytes
> +Date:		February 2024
> +Contact:	Himanshu Madhani <himanshu.madhani@oracle.com>
> +Description:
> +		[RO] This parameter specifies the maximum atomic write
> +		size reported by the device. This parameter is relevant
> +		for merging of writes, where a merged atomic write
> +		operation must not exceed this number of bytes.
> +		This parameter may be greater than the value in
> +		atomic_write_unit_max_bytes as
> +		atomic_write_unit_max_bytes will be rounded down to a
> +		power-of-two and atomic_write_unit_max_bytes may also be
> +		limited by some other queue limits, such as max_segments.
> +		This parameter - along with atomic_write_unit_min_bytes
> +		and atomic_write_unit_max_bytes - will not be larger than
> +		max_hw_sectors_kb, but may be larger than max_sectors_kb.
> +
> +
> +What:		/sys/block/<disk>/atomic_write_unit_min_bytes
> +Date:		February 2024
> +Contact:	Himanshu Madhani <himanshu.madhani@oracle.com>
> +Description:
> +		[RO] This parameter specifies the smallest block which can
> +		be written atomically with an atomic write operation. All
> +		atomic write operations must begin at a
> +		atomic_write_unit_min boundary and must be multiples of
> +		atomic_write_unit_min. This value must be a power-of-two.
> +
> +
> +What:		/sys/block/<disk>/atomic_write_unit_max_bytes
> +Date:		February 2024
> +Contact:	Himanshu Madhani <himanshu.madhani@oracle.com>
> +Description:
> +		[RO] This parameter defines the largest block which can be
> +		written atomically with an atomic write operation. This
> +		value must be a multiple of atomic_write_unit_min and must
> +		be a power-of-two. This value will not be larger than
> +		atomic_write_max_bytes.
> +
> +
> +What:		/sys/block/<disk>/atomic_write_boundary_bytes
> +Date:		February 2024
> +Contact:	Himanshu Madhani <himanshu.madhani@oracle.com>
> +Description:
> +		[RO] A device may need to internally split an atomic write I/O
> +		which straddles a given logical block address boundary. This
> +		parameter specifies the size in bytes of the atomic boundary if
> +		one is reported by the device. This value must be a
> +		power-of-two and at least the size as in
> +		atomic_write_unit_max_bytes.
> +		Any attempt to merge atomic write I/Os must not result in a
> +		merged I/O which crosses this boundary (if any).
> +
>   
>   What:		/sys/block/<disk>/diskseq
>   Date:		February 2021
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 82c3ae22d76d..d9f58fe71758 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -174,6 +174,8 @@ static const struct {
>   	/* Command duration limit device-side timeout */
>   	[BLK_STS_DURATION_LIMIT]	= { -ETIME, "duration limit exceeded" },
>   
> +	[BLK_STS_INVAL]		= { -EINVAL,	"invalid" },
> +
>   	/* everything else not covered above: */
>   	[BLK_STS_IOERR]		= { -EIO,	"I/O" },
>   };
> @@ -739,6 +741,18 @@ void submit_bio_noacct_nocheck(struct bio *bio)
>   		__submit_bio_noacct(bio);
>   }
>   
> +static blk_status_t blk_validate_atomic_write_op_size(struct request_queue *q,
> +						 struct bio *bio)
> +{
> +	if (bio->bi_iter.bi_size > queue_atomic_write_unit_max_bytes(q))
> +		return BLK_STS_INVAL;
> +
> +	if (bio->bi_iter.bi_size % queue_atomic_write_unit_min_bytes(q))
> +		return BLK_STS_INVAL;
> +
> +	return BLK_STS_OK;
> +}
> +
>   /**
>    * submit_bio_noacct - re-submit a bio to the block device layer for I/O
>    * @bio:  The bio describing the location in memory and on the device.
> @@ -797,6 +811,11 @@ void submit_bio_noacct(struct bio *bio)
>   	switch (bio_op(bio)) {
>   	case REQ_OP_READ:
>   	case REQ_OP_WRITE:
> +		if (bio->bi_opf & REQ_ATOMIC) {
> +			status = blk_validate_atomic_write_op_size(q, bio);
> +			if (status != BLK_STS_OK)
> +				goto end_io;
> +		}
>   		break;
>   	case REQ_OP_FLUSH:
>   		/*
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 8957e08e020c..ad07759ca147 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -18,6 +18,46 @@
>   #include "blk-rq-qos.h"
>   #include "blk-throttle.h"
>   
> +/*
> + * rq_straddles_atomic_write_boundary - check for boundary violation
> + * @rq: request to check
> + * @front: data size to be appended to front
> + * @back: data size to be appended to back
> + *
> + * Determine whether merging a request or bio into another request will result
> + * in a merged request which straddles an atomic write boundary.
> + *
> + * The value @front_adjust is the data which would be appended to the front of
> + * @rq, while the value @back_adjust is the data which would be appended to the
> + * back of @rq. Callers will typically only have either @front_adjust or
> + * @back_adjust as non-zero.
> + *
> + */
> +static bool rq_straddles_atomic_write_boundary(struct request *rq,
> +					unsigned int front_adjust,
> +					unsigned int back_adjust)
> +{
> +	unsigned int boundary = queue_atomic_write_boundary_bytes(rq->q);
> +	u64 mask, start_rq_pos, end_rq_pos;
> +
> +	if (!boundary)
> +		return false;
> +
> +	start_rq_pos = blk_rq_pos(rq) << SECTOR_SHIFT;
> +	end_rq_pos = start_rq_pos + blk_rq_bytes(rq) - 1;
> +
> +	start_rq_pos -= front_adjust;
> +	end_rq_pos += back_adjust;
> +
> +	mask = ~(boundary - 1);
> +
> +	/* Top bits are different, so crossed a boundary */
> +	if ((start_rq_pos & mask) != (end_rq_pos & mask))
> +		return true;
> +
> +	return false;
> +}

But isn't that precisely what 'chunk_sectors' is doing?
IE ensuring that requests never cross that boundary?

Q1: Shouldn't we rather use/modify/adapt chunk_sectors for this thing?
Q2: If we don't, shouldn't we align the atomic write boundary to the 
chunk_sectors setting to ensure both match up?

Cheers,

Hannes