Re: [PATCH 10/21] block: Add fops atomic write support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux