Re: [PATCH v7 1/7] block: Introduce the use_zone_write_lock member variable

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

 



On 8/10/23 17:39, Damien Le Moal wrote:
On 8/10/23 23:02, Bart Van Assche wrote:
My concerns about the above proposal are as follows:
* The information about whether or not the zone write lock should be used comes
    from the block driver, e.g. a SCSI LLD.

Yes.

* sdp, the SCSI disk pointer, is owned by the ULD.
* An ULD may be attached and detached multiple times during the lifetime of a
    logical unit without the LLD being informed about this. So how to set
    sdp->use_zone_write_lock without introducing a new callback or member variable
    in a data structure owned by the LLD?

That would be set during device scan and device revalidate. And if the value
changes, then disk_set_zoned() should be called again to update the queue limit.
That is already what is done for the zoned limit indicating the type of the
drive. My point is that the zoned limit should dictate if use_zone_write_lock
can be true. The default should be be:

	q->limits.use_zone_write_lock = q->limits.zoned != BLK_ZONED_NONE.

And as proposed, if the UFS driver wants to disable zone write locking, all it
needs to do is call "disk_set_zoned(disk, BLK_ZONED_HM, false)". I did not try
to actually code that, and the scsi disk driver may be in the way and that may
need to be done there, hence the suggestion of having a use_zone_write_lock flag
in the scsi device structure so that the UFS driver can set it as needed as well
(and detect changes when revalidating). That should work, but I may be missing
something.

Hi Damien,

Adding a use_zone_write_lock argument to disk_set_zoned() seems wrong to me. The information about whether or not to use the zone write lock comes from the LLD. The zone model is retrieved by the ULD. Since both pieces of information come from different drivers, both properties should be modified independently.

Moving the use_zone_write_lock member variable into a data structure owned by the ULD seems wrong to me because that member variable is set by the LLD.

Reviewers are allowed to request changes for well-designed and working code but should be able to explain why they request these changes. Can you please explain why you care about the value of q->limits.use_zone_write_lock for the BLK_ZONED_NONE case? If dd_use_zone_write_locking() would be renamed and would be moved into include/linux/blkdev.h and if reads of q->limits.use_zone_write_lock would be changed into blk_use_zone_write_locking() calls then the value of q->limits.use_zone_write_lock for the BLK_ZONED_NONE case wouldn't matter at all.

Thanks,

Bart.



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux