On Sat, Nov 19, 2022 at 09:24:01AM +0900, Damien Le Moal wrote: > On 11/18/22 21:47, Ming Lei wrote: > > On Fri, Nov 18, 2022 at 12:49:15PM +0100, Andreas Hindborg wrote: > >> > >> Ming Lei <ming.lei@xxxxxxxxxx> writes: > >> > >>> CAUTION: This email originated from outside of Western Digital. Do not click on > >>> links or open attachments unless you recognize the sender and know that the > >>> content is safe. > >>> > >>> > >>> On Fri, Nov 18, 2022 at 10:41:31AM +0100, Andreas Hindborg wrote: > >>>> > >>>> Ming Lei <ming.lei@xxxxxxxxxx> writes: > >>>> > >>>>> CAUTION: This email originated from outside of Western Digital. Do not click on > >>>>> links or open attachments unless you recognize the sender and know that the > >>>>> content is safe. > >>>>> > >>>>> > >>>>> On Fri, Nov 18, 2022 at 01:35:29PM +0900, Damien Le Moal wrote: > >>>>>> On 11/18/22 13:12, Ming Lei wrote: > >>>>>> [...] > >>>>>>>>> You can only assign it to zoned write request, but you still have to check > >>>>>>>>> the sequence inside each zone, right? Then why not just check LBAs in > >>>>>>>>> each zone simply? > >>>>>>>> > >>>>>>>> We would need to know the zone map, which is not otherwise required. > >>>>>>>> Then we would need to track the write pointer for each open zone for > >>>>>>>> each queue, so that we can stall writes that are not issued at the write > >>>>>>>> pointer. This is in effect all zones, because we cannot track when zones > >>>>>>>> are implicitly closed. Then, if different queues are issuing writes to > >>>>>>> > >>>>>>> Can you explain "implicitly closed" state a bit? > >>>>>>> > >>>>>>> From https://zonedstorage.io/docs/introduction/zoned-storage, only the > >>>>>>> following words are mentioned about closed state: > >>>>>>> > >>>>>>> ```Conversely, implicitly or explicitly opened zoned can be transitioned to the > >>>>>>> closed state using the CLOSE ZONE command.``` > >>>>>> > >>>>>> When a write is issued to an empty or closed zone, the drive will > >>>>>> automatically transition the zone into the implicit open state. This is > >>>>>> called implicit open because the host did not (explicitly) issue an open > >>>>>> zone command. > >>>>>> > >>>>>> When there are too many implicitly open zones, the drive may choose to > >>>>>> close one of the implicitly opened zone to implicitly open the zone that > >>>>>> is a target for a write command. > >>>>>> > >>>>>> Simple in a nutshell. This is done so that the drive can work with a > >>>>>> limited set of resources needed to handle open zones, that is, zones that > >>>>>> are being written. There are some more nasty details to all this with > >>>>>> limits on the number of open zones and active zones that a zoned drive may > >>>>>> have. > >>>>> > >>>>> OK, thanks for the clarification about implicitly closed, but I > >>>>> understand this close can't change the zone's write pointer. > >>>> > >>>> You are right, it does not matter if the zone is implicitly closed, I > >>>> was mistaken. But we still have to track the write pointer of every zone > >>>> in open or active state, otherwise we cannot know if a write that arrive > >>>> to a zone with no outstanding IO is actually at the write pointer, or > >>>> whether we need to hold it. > >>>> > >>>>> > >>>>>> > >>>>>>> > >>>>>>> zone info can be cached in the mapping(hash table)(zone sector is the key, and zone > >>>>>>> info is the value), which can be implemented as one LRU style. If any zone > >>>>>>> info isn't hit in the mapping table, ioctl(BLKREPORTZONE) can be called for > >>>>>>> obtaining the zone info. > >>>>>>> > >>>>>>>> the same zone, we need to sync across queues. Userspace may have > >>>>>>>> synchronization in place to issue writes with multiple threads while > >>>>>>>> still hitting the write pointer. > >>>>>>> > >>>>>>> You can trust mq-dealine, which guaranteed that write IO is sent to ->queue_rq() > >>>>>>> in order, no matter MQ or SQ. > >>>>>>> > >>>>>>> Yes, it could be issue from multiple queues for ublksrv, which doesn't sync > >>>>>>> among multiple queues. > >>>>>>> > >>>>>>> But per-zone re-order still can solve the issue, just need one lock > >>>>>>> for each zone to cover the MQ re-order. > >>>>>> > >>>>>> That lock is already there and using it, mq-deadline will never dispatch > >>>>>> more than one write per zone at any time. This is to avoid write > >>>>>> reordering. So multi queue or not, for any zone, there is no possibility > >>>>>> of having writes reordered. > >>>>> > >>>>> oops, I miss the single queue depth point per zone, so ublk won't break > >>>>> zoned write at all, and I agree order of batch IOs is one problem, but > >>>>> not hard to solve. > >>>> > >>>> The current implementation _does_ break zoned write because it reverses > >>>> batched writes. But if it is an easy fix, that is cool :) > >>> > >>> Please look at Damien's comment: > >>> > >>>>> That lock is already there and using it, mq-deadline will never dispatch > >>>>> more than one write per zone at any time. This is to avoid write > >>>>> reordering. So multi queue or not, for any zone, there is no possibility > >>>>> of having writes reordered. > >>> > >>> For zoned write, mq-deadline is used to limit at most one inflight write > >>> for each zone. > >>> > >>> So can you explain a bit how the current implementation breaks zoned > >>> write? > >> > >> Like Damien wrote in another email, mq-deadline will only impose > >> ordering for requests submitted in batch. The flow we have is the > >> following: > >> > >> - Userspace sends requests to ublk gendisk > >> - Requests go through block layer and is _not_ reordered when using > >> mq-deadline. They may be split. > >> - Requests hit ublk_drv and ublk_drv will reverse order of _all_ > >> batched up requests (including split requests). > > > > For ublk-zone, ublk driver needs to be exposed as zoned device by > > calling disk_set_zoned() finally, which definitely isn't supported now, > > so mq-deadline at most sends one write IO for each zone after ublk-zone > > is supported, see blk_req_can_dispatch_to_zone(). > > > >> - ublk_drv sends request to ublksrv in _reverse_ order. > >> - ublksrv sends requests _not_ batched up to target device. > >> - Requests that enter mq-deadline at the same time are reordered in LBA > >> order, that is all good. > >> - Requests that enter the kernel in different batches are not reordered > >> in LBA order and end up missing the write pointer. This is bad. > > > > Again, please read Damien's comment: > > > >>> That lock is already there and using it, mq-deadline will never dispatch > >>> more than one write per zone at any time. > > > > Anytime, there is at most one write IO for each zone, how can the single > > write IO be re-order? > > If the user issues writes one at a time out of order (not aligned to the > write pointer), mq-deadline will not help at all. The zone write locking > will still limit write dispatching to one per zone, but the writes will fail. > > mq-deadline will reorder write commands in the correct lba order only if: > - the commands are inserted as a batch (more than on request passed to > ->insert_requests) > - commands are inserted individually when the target zone is locked (a > write is already being executed) > > This has been the semantic from the start: the block layer has no > guarantees about the correct ordering of writes to zoned drive. What is > guaranteed is that (1) if the user issues writes in order AND (2) > mq-deadline is used, then writes will be dispatched in the same order to > the device. > > I have not looked at the details of ublk, but from the thread, I think (1) > is not done and (2) is missing-ish as the ublk device is not marked as zoned. (2) is supposed to be done for support ublk-zone, at least in the Andreas's PR, which just implements one ublk-loop over one real backing zoned disk. mq-deadline dispatches one write IO for each zone on /dev/ublkbN( supposed to be exposed as zoned, not done yet), and this write IO will be forwarded to ublksrv, so ublksrv can only see one such write IO for each zone, which will be re-issued to the backing real zoned disk, so there can't be the write io reorder issue, cause the write batch just has one write IO for each zone. [1] https://github.com/ming1/ubdsrv/pull/28/files Thanks, Ming
