Re: [PATCHv5 3/7] block: remove q->sysfs_lock for attributes which don't need it

[Ming Lei <ming.lei@redhat.com>]

On Wed, Feb 26, 2025 at 06:09:56PM +0530, Nilay Shroff wrote:
> There're few sysfs attributes in block layer which don't really need
> acquiring q->sysfs_lock while accessing it. The reason being, reading/
> writing a value from/to such attributes are either atomic or could be
> easily protected using READ_ONCE()/WRITE_ONCE(). Moreover, sysfs
> attributes are inherently protected with sysfs/kernfs internal locking.
> 
> So this change help segregate all existing sysfs attributes for which
> we could avoid acquiring q->sysfs_lock. For all read-only attributes
> we removed the q->sysfs_lock from show method of such attributes. In
> case attribute is read/write then we removed the q->sysfs_lock from
> both show and store methods of these attributes.
> 
> We audited all block sysfs attributes and found following list of
> attributes which shouldn't require q->sysfs_lock protection:
> 
> 1. io_poll:
>    Write to this attribute is ignored. So, we don't need q->sysfs_lock.
> 
> 2. io_poll_delay:
>    Write to this attribute is NOP, so we don't need q->sysfs_lock.
> 
> 3. io_timeout:
>    Write to this attribute updates q->rq_timeout and read of this
>    attribute returns the value stored in q->rq_timeout Moreover, the
>    q->rq_timeout is set only once when we init the queue (under blk_mq_
>    init_allocated_queue()) even before disk is added. So that means
>    that we don't need to protect it with q->sysfs_lock. As this
>    attribute is not directly correlated with anything else simply using
>    READ_ONCE/WRITE_ONCE should be enough.
> 
> 4. nomerges:
>    Write to this attribute file updates two q->flags : QUEUE_FLAG_
>    NOMERGES and QUEUE_FLAG_NOXMERGES. These flags are accessed during
>    bio-merge which anyways doesn't run with q->sysfs_lock held.
>    Moreover, the q->flags are updated/accessed with bitops which are
>    atomic. So, protecting it with q->sysfs_lock is not necessary.
> 
> 5. rq_affinity:
>    Write to this attribute file makes atomic updates to q->flags:
>    QUEUE_FLAG_SAME_COMP and QUEUE_FLAG_SAME_FORCE. These flags are
>    also accessed from blk_mq_complete_need_ipi() using test_bit macro.
>    As read/write to q->flags uses bitops which are atomic, protecting
>    it with q->stsys_lock is not necessary.
> 
> 6. nr_zones:
>    Write to this attribute happens in the driver probe method (except
>    nvme) before disk is added and outside of q->sysfs_lock or any other
>    lock. Moreover nr_zones is defined as "unsigned int" and so reading
>    this attribute, even when it's simultaneously being updated on other
>    cpu, should not return torn value on any architecture supported by
>    linux. So we can avoid using q->sysfs_lock or any other lock/
>    protection while reading this attribute.
> 
> 7. discard_zeroes_data:
>    Reading of this attribute always returns 0, so we don't require
>    holding q->sysfs_lock.
> 
> 8. write_same_max_bytes
>    Reading of this attribute always returns 0, so we don't require
>    holding q->sysfs_lock.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>

Reviewed-by: Ming Lei <ming.lei@redhat.com>


Thanks,
Ming