On 04/12/2023 02:30, Ming Lei wrote:
Hi Ming,
+static bool blkdev_atomic_write_valid(struct block_device *bdev, loff_t pos,
+ struct iov_iter *iter)
+{
+ unsigned int atomic_write_unit_min_bytes =
+ queue_atomic_write_unit_min_bytes(bdev_get_queue(bdev));
+ unsigned int atomic_write_unit_max_bytes =
+ queue_atomic_write_unit_max_bytes(bdev_get_queue(bdev));
+
+ if (!atomic_write_unit_min_bytes)
+ return false;
The above check should have be moved to limit setting code path.
Sorry, I didn't fully understand your point.
I added this here (as opposed to the caller), as I was not really
worried about speeding up the failure path. Are you saying to call even
earlier in submission path?
+ if (pos % atomic_write_unit_min_bytes)
+ return false;
+ if (iov_iter_count(iter) % atomic_write_unit_min_bytes)
+ return false;
+ if (!is_power_of_2(iov_iter_count(iter)))
+ return false;
+ if (iov_iter_count(iter) > atomic_write_unit_max_bytes)
+ return false;
+ if (pos % iov_iter_count(iter))
+ return false;
I am a bit confused about relation between atomic_write_unit_max_bytes and
atomic_write_max_bytes.
I think that naming could be improved. Or even just drop merging (and
atomic_write_max_bytes concept) until we show it to improve performance.
So generally atomic_write_unit_max_bytes will be same as
atomic_write_max_bytes, however it could be different if:
a. request queue nr hw segments or other request queue limits needs to
restrict atomic_write_unit_max_bytes
b. atomic_write_unit_max_bytes does not need to be a power-of-2 and
atomic_write_max_bytes does. So essentially:
atomic_write_unit_max_bytes = rounddown_pow_of_2(atomic_write_max_bytes)
Here the max IO length is limited to be <= atomic_write_unit_max_bytes,
so looks userspace can only submit IO with write-atomic-unit naturally
aligned IO(such as, 4k, 8k, 16k, 32k, ...),
correct
but these user IOs are
allowed to be merged to big one if naturally alignment is respected and
the merged IO size is <= atomic_write_max_bytes.
correct, but the resultant merged IO does not have have to be naturally
aligned.
Is my understanding right?
Yes, but...
If yes, I'd suggest to document the point,
and the last two checks could be change to:
/* naturally aligned */
if (pos % iov_iter_count(iter))
return false;
if (iov_iter_count(iter) > atomic_write_max_bytes)
return false;
.. we would not be merging at this point as this is just IO submission
to the block layer, so atomic_write_max_bytes does not come into play
yet. If you check patch 7/21, you will see that we limit IO size to
atomic_write_max_bytes, which is relevant merging.
Thanks,
John
