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

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

 



On 2/24/25 14:30, 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.

That's not the point; point is that the queue is frozen when updating so
blk-merge will never see an incomplete update.
But that's a minor issue and doesn't affect the patch itself.

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@xxxxxx>
Signed-off-by: Nilay Shroff <nilay@xxxxxxxxxxxxx>
---
  block/blk-settings.c |  2 +-
  block/blk-sysfs.c    | 81 +++++++++++++++-----------------------------
  2 files changed, 29 insertions(+), 54 deletions(-)

Reviewed-by: Hannes Reinecke <hare@xxxxxxx>

Cheers,

Hannes
--
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@xxxxxxx                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich




[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