Message-ID: <fc91aa12-1707-9825-a77e-9d5a41d97808@oracle.com>
Date:   Thu, 4 May 2023 19:14:29 +0100
From:   John Garry <john.g.garry@...cle.com>
To:     Dave Chinner <david@...morbit.com>
Cc:     axboe@...nel.dk, kbusch@...nel.org, hch@....de, sagi@...mberg.me,
        martin.petersen@...cle.com, djwong@...nel.org,
        viro@...iv.linux.org.uk, brauner@...nel.org, dchinner@...hat.com,
        jejb@...ux.ibm.com, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-nvme@...ts.infradead.org,
        linux-scsi@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        linux-security-module@...r.kernel.org, paul@...l-moore.com,
        jmorris@...ei.org, serge@...lyn.com,
        Himanshu Madhani <himanshu.madhani@...cle.com>
Subject: Re: [PATCH RFC 01/16] block: Add atomic write operations to
 request_queue limits

Hi Dave,

>> diff --git a/Documentation/ABI/stable/sysfs-block b/Documentation/ABI/stable/sysfs-block
>> index 282de3680367..f3ed9890e03b 100644
>> --- a/Documentation/ABI/stable/sysfs-block
>> +++ b/Documentation/ABI/stable/sysfs-block
>> @@ -21,6 +21,48 @@ Description:
>>   		device is offset from the internal allocation unit's
>>   		natural alignment.
>>   
>> +What:		/sys/block/<disk>/atomic_write_max_bytes
>> +Date:		May 2023
>> +Contact:	Himanshu Madhani <himanshu.madhani@...cle.com>
>> +Description:
>> +		[RO] This parameter specifies the maximum atomic write
>> +		size reported by the device. An atomic write operation
>> +		must not exceed this number of bytes.
>> +
>> +
>> +What:		/sys/block/<disk>/atomic_write_unit_min
>> +Date:		May 2023
>> +Contact:	Himanshu Madhani <himanshu.madhani@...cle.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 units is this defined to use? Bytes?

Bytes

> 
>> +
>> +
>> +What:		/sys/block/<disk>/atomic_write_unit_max
>> +Date:		January 2023
>> +Contact:	Himanshu Madhani <himanshu.madhani@...cle.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.
> 
> Same question. Also, how is this different to
> atomic_write_max_bytes?

Again, this is bytes. We can add "bytes" to the name of these other 
files if people think it's better. Unfortunately request_queue sysfs 
file naming isn't consistent here to begin with.

atomic_write_unit_max is largest application block size which we can 
support, while atomic_write_max_bytes is the max size of an atomic 
operation which the HW supports.

 From your review on the iomap patch, I assume that now you realise that 
we are proposing a write which may include multiple application data 
blocks (each limited in size to atomic_write_unit_max), and the limit in 
total size of that write is atomic_write_max_bytes.

user applications should only pay attention to what we return from 
statx, that being atomic_write_unit_min and atomic_write_unit_max.

atomic_write_max_bytes and atomic_write_boundary is only relevant to the 
block layer.

> 
>> +
>> +
>> +What:		/sys/block/<disk>/atomic_write_boundary
>> +Date:		May 2023
>> +Contact:	Himanshu Madhani <himanshu.madhani@...cle.com>
>> +Description:
>> +		[RO] A device may need to internally split I/Os which
>> +		straddle a given logical block address boundary. In that
>> +		case a single atomic write operation will be processed as
>> +		one of more sub-operations which each complete atomically.
>> +		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.
> 
> How are users/filesystems supposed to use this?

As above, this is not relevant to the user.

> 
>> +
>>   
>>   What:		/sys/block/<disk>/diskseq
>>   Date:		February 2021
>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>> index 896b4654ab00..e21731715a12 100644
>> --- a/block/blk-settings.c
>> +++ b/block/blk-settings.c
>> @@ -59,6 +59,9 @@ void blk_set_default_limits(struct queue_limits *lim)
>>   	lim->zoned = BLK_ZONED_NONE;
>>   	lim->zone_write_granularity = 0;
>>   	lim->dma_alignment = 511;
>> +	lim->atomic_write_unit_min = lim->atomic_write_unit_max = 1;
> 
> A value of "1" isn't obviously a power of 2, nor does it tell me
> what units these values use.

I think that we should store these in bytes.

> 
>> +	lim->atomic_write_max_bytes = 512;
>> +	lim->atomic_write_boundary = 0;
> 
> The behaviour when the value is zero is not defined by the syfs
> description above.

I'll add it. A value of zero means no atomic boundary.

> 
>>   }
>>   
>>   /**
>> @@ -183,6 +186,59 @@ void blk_queue_max_discard_sectors(struct request_queue *q,
>>   }
>>   EXPORT_SYMBOL(blk_queue_max_discard_sectors);
>>   
>> +/**
>> + * blk_queue_atomic_write_max_bytes - set max bytes supported by
>> + * the device for atomic write operations.
>> + * @q:  the request queue for the device
>> + * @size: maximum bytes supported
>> + */
>> +void blk_queue_atomic_write_max_bytes(struct request_queue *q,
>> +				      unsigned int size)
>> +{
>> +	q->limits.atomic_write_max_bytes = size;
>> +}
>> +EXPORT_SYMBOL(blk_queue_atomic_write_max_bytes);
>> +
>> +/**
>> + * blk_queue_atomic_write_boundary - Device's logical block address space
>> + * which an atomic write should not cross.
> 
> I have no idea what "logical block address space which an atomic
> write should not cross" means, especially as the unit is in bytes
> and not in sectors (which are the units LBAs are expressed in).

It means that an atomic operation which straddles the atomic boundary is 
not guaranteed to be atomic by the device, so we should (must) not cross 
it to maintain atomic behaviour for an application block. That's one 
reason that we have all these size and alignment rules.

> 
>> + * @q:  the request queue for the device
>> + * @size: size in bytes. Must be a power-of-two.
>> + */
>> +void blk_queue_atomic_write_boundary(struct request_queue *q,
>> +				     unsigned int size)
>> +{
>> +	q->limits.atomic_write_boundary = size;
>> +}
>> +EXPORT_SYMBOL(blk_queue_atomic_write_boundary);
>> +
>> +/**
>> + * blk_queue_atomic_write_unit_min - smallest unit that can be written
>> + *				     atomically to the device.
>> + * @q:  the request queue for the device
>> + * @sectors: must be a power-of-two.
>> + */
>> +void blk_queue_atomic_write_unit_min(struct request_queue *q,
>> +				     unsigned int sectors)
>> +{
>> +	q->limits.atomic_write_unit_min = sectors;
>> +}
>> +EXPORT_SYMBOL(blk_queue_atomic_write_unit_min);
> 
> Oh, these are sectors?

Again, we'll change to bytes.

> 
> What size sector? Are we talking about fixed size 512 byte basic
> block units,

Normally we would be referring to fixed size 512 byte basic
block unit

> or are we talking about physical device sector sizes
> (e.g. 4kB, maybe larger in future?)
> 
> These really should be in bytes, as they are directly exposed to
> userspace applications via statx and applications will have no idea
> what the sector size actually is without having to query the block
> device directly...

ok

> 
>> +
>> +/*
>> + * blk_queue_atomic_write_unit_max - largest unit that can be written
>> + * atomically to the device.
>> + * @q: the reqeust queue for the device
>> + * @sectors: must be a power-of-two.
>> + */
>> +void blk_queue_atomic_write_unit_max(struct request_queue *q,
>> +				     unsigned int sectors)
>> +{
>> +	struct queue_limits *limits = &q->limits;
>> +	limits->atomic_write_unit_max = sectors;
>> +}
>> +EXPORT_SYMBOL(blk_queue_atomic_write_unit_max);
>> +
>>   /**
>>    * blk_queue_max_secure_erase_sectors - set max sectors for a secure erase
>>    * @q:  the request queue for the device
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index f1fce1c7fa44..1025beff2281 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -132,6 +132,30 @@ static ssize_t queue_max_discard_segments_show(struct request_queue *q,
>>   	return queue_var_show(queue_max_discard_segments(q), page);
>>   }
>>   

...

>>   
>> +static inline unsigned int queue_atomic_write_unit_max(const struct request_queue *q)
>> +{
>> +	return q->limits.atomic_write_unit_max << SECTOR_SHIFT;
>> +}
>> +
>> +static inline unsigned int queue_atomic_write_unit_min(const struct request_queue *q)
>> +{
>> +	return q->limits.atomic_write_unit_min << SECTOR_SHIFT;
>> +}
> 
> Ah, what? This undocumented interface reports "unit limits" in
> bytes, but it's not using the physical device sector size to convert
> between sector units and bytes. This really needs some more
> documentation and work to make it present all units consistently and
> not result in confusion when devices have 4kB sector sizes and not
> 512 byte sectors...

ok, we'll look to fix this up to give a coherent and clear interface.

> 
> Also, I think all the byte ranges should support full 64 bit values,
> otherwise there will be silent overflows in converting 32 bit sector
> counts to byte ranges. And, eventually, something will want to do
> larger than 4GB atomic IOs
> 

ok, we can do that but would also then make statx field 64b. I'm fine 
with that if it is wise to do so - I don't don't want to wastefully use 
up an extra 2 x 32b in struct statx.

Thanks,
John

Powered by blists - more mailing lists