On 8/12/23 00:41, Bart Van Assche wrote: > 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. OK. But I still think that disk_set_zoned() is the place where the default for use_zone_write_lock should be set. And we need a clean way for the LLD to change use_zone_write_lock. > > 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 Because use_zone_write_lock should ever be true for !BLK_ZONED_NONE case. Allowing use_zone_write_lock to be true even with zoned == BLK_ZONED_NONE forces to always check that the device is zoned to ensure that the value of use_zone_write_lock is valid. This is awckward and uselessly complicate things. > 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. Sure, that would work. But again, why the need to check both model and actual value of use_zone_write_lock ? I do not think it is that hard to keep use_zone_write_lock to false for the BLK_ZONED_NONE case. Then blk_use_zone_write_locking() is reduced to returning the value of use_zone_write_lock. > > Thanks, > > Bart. -- Damien Le Moal Western Digital Research
