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.
