Re: [PATCH 2/4] generic/765: Fix sysfs path for nvme partitions

On Sun, Apr 05, 2026 at 08:37:12PM +0530, Ojaswin Mujoo wrote:
> On Wed, Apr 01, 2026 at 07:08:29AM -0700, Darrick J. Wong wrote:
> > On Wed, Apr 01, 2026 at 04:10:48PM +0530, Ojaswin Mujoo wrote:
> > > This tests checks atomic write limits reported by statx() are same as
> > > the ones reported by sysfs, however nvme partitions don't have the
> > > /sys/block/nvme0n1p1 style entry and also use the namespace's queue
> > > limits for atomic writes. This causes the test to fail because it
> > > expects /sys/block/nvme0n*p* to be present.
> > > 
> > > Hence, in this case, just check against the parent namespace.
> > > 
> > > Reported-by: Disha Goel <[email protected]>
> > > Signed-off-by: Ojaswin Mujoo <[email protected]>
> > > ---
> > >  tests/generic/765 | 23 +++++++++++++++++++----
> > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/generic/765 b/tests/generic/765
> > > index 8c4e0bd0..4c768783 100755
> > > --- a/tests/generic/765
> > > +++ b/tests/generic/765
> > > @@ -94,16 +94,31 @@ test_atomic_writes()
> > >      _scratch_unmount
> > >  }
> > >  
> > > -sys_min_write=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/atomic_write_unit_min_bytes")
> > > -sys_max_write=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/atomic_write_unit_max_bytes")
> > 
> > I see this show up in a few other places:
> > 
> > $ git grep /sys/block
> > common/dmlogwrites:64:  local sysfs="/sys/block/$(_short_dev $1)"
> > common/rc:2567: local sysfs="/sys/block/$(_short_dev $SCRATCH_DEV)"
> > common/rc:2631: if [ -e /sys/block/${sdev}/queue/zoned ]; then
> > common/rc:2632:         cat /sys/block/${sdev}/queue/zoned
> > common/scsi_debug:67:   device=`grep -wl scsi_debug /sys/block/sd*/device/model | awk -F / '{print $4}'`
> > tests/btrfs/076:31:     zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes")
> > tests/btrfs/237:48:zone_size=$(($(cat /sys/block/${sdev}/queue/chunk_sectors) << 9))
> > tests/btrfs/283:29:     zone_append_max=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/zone_append_max_bytes")
> > vests/generic/108:20:   echo running > /sys/block/`_short_dev $SCSI_DEBUG_DEV`/device/state
> > tests/generic/108:81:echo offline > /sys/block/`_short_dev $SCSI_DEBUG_DEV`/device/state
> > tests/generic/730:56:echo 1 > /sys/block/$(_short_dev $SCSI_DEBUG_DEV)/device/delete
> > tests/generic/731:49:echo 1 > /sys/block/`_short_dev $SCSI_DEBUG_DEV`/device/delete
> > tests/generic/765:97:sys_min_write=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/atomic_write_unit_min_bytes")
> > tests/generic/765:98:sys_max_write=$(cat "/sys/block/$(_short_dev $SCRATCH_DEV)/queue/atomic_write_unit_max_bytes")
> > 
> > So I wonder if the same problem exists there as well?
> 
> Okay so seems like there general rule in the kernel is:
> 
> File: block/partitions/core.c:336-339
> 
> dname = dev_name(ddev);
> if (isdigit(dname[strlen(dname) - 1]))
>     dev_set_name(pdev, "%sp%d", dname, partno);
> else
>     dev_set_name(pdev, "%s%d", dname, partno);
> 
> So any device where names end with numbers follow the name<%d>p<%d>
> scheme for partitions.

Eeeeeee thanks for digging that out :)

> I was not completely sure if this is something only nvmes follow but
> looking into the code a bit (from my limited understanding of the blkdev
> code) seems like all  partiions just inherit parent's queues and sysfs
> "queue" is only for the parent (struct gendisk) rather than partiions.

Yes, partitions just reuse the parent bdev's queues.

> So I think a more generic way would be to check if dev is a partitions
> and then get the correct path for it:
> 
> dev_is_partition() {
>     local dev
>     dev=$(_short_dev "$1")
> 
>     [[ -e /sys/class/block/$dev/partition ]]
> }
> 
> dev_get_queue_path() {
>     local dev parent
>     dev=$(_short_dev "$1")
> 
>     if dev_is_partition "$dev"; then
>         parent=$(basename "$(readlink -f /sys/class/block/$dev/..)")

Heh, that '..' is delectably subtle.

>     else
>         parent="$dev"
>     fi
> 
>     echo "/sys/block/$parent/queue"

Might want to check that the path actually exists before printing it,
but yes that looks ok.

--D

> }
> 
> Does that look okay?
> 
> Regards,
> ojaswin
> 
> > 
> > (Well, maybe not the scsi-debug users)
> > 
> > > +# Get the right sysfs dev for the atomic write device
> > > +get_aw_dev() {
> > > +    local dev
> > > +    dev=$(_short_dev "$1")
> > > +
> > > +    # Special case: nvme partitions don't have the /sys/block/nvme0n1p1 style
> > > +    # entry and also use the namespace's queue limits for atomic writes, hence
> > > +    # just return the parent namespace.
> > > +    if [[ $dev == nvme* ]]; then
> > > +        echo "${dev%%p[0-9]*}"
> > > +    else
> > > +        echo "$dev"
> > > +    fi
> > > +}
> > 
> > Perhaps this ought to turn into a common helper to fix them all?
> > 
> > _sys_block_path() {
> > 	local dev=$(_short_dev "$1")
> > 
> > 	# Special case: nvme partitions don't have the /sys/block/nvme0n1p1 style
> > 	# entry and also use the namespace's queue limits for atomic writes, hence
> > 	# just return the parent namespace.
> > 	if [[ $dev == nvme* ]]; then
> > 		echo "/sys/block/${dev%%p[0-9]*}"
> > 	else
> > 		echo "/sys/block/$dev"
> > 	fi
> > }
> > 
> > sys_min_write=$(_sys_block_path $SCRATCH_DEV)/queue/<whatever>)
> > 
> > Also, don't some of the other block devices follow this "partition has
> > pXXX suffix" pattern as well?  Not that I have a clue why scsi doesn't
> > follow that pattern or even which part of sd.c triggers the sdaX
> > behavior. :/
> > 
> > --D
> > 
> > > +
> > > +sys_min_write=$(cat "/sys/block/$(get_aw_dev $SCRATCH_DEV)/queue/atomic_write_unit_min_bytes")
> > > +sys_max_write=$(cat "/sys/block/$(get_aw_dev $SCRATCH_DEV)/queue/atomic_write_unit_max_bytes")
> > >  
> > >  bdev_min_write=$(_get_atomic_write_unit_min $SCRATCH_DEV)
> > >  bdev_max_write=$(_get_atomic_write_unit_max $SCRATCH_DEV)
> > >  
> > >  echo "sysfs awu_min $sys_min_write" >> $seqres.full
> > > -echo "sysfs awu_min $sys_max_write" >> $seqres.full
> > > +echo "sysfs awu_max $sys_max_write" >> $seqres.full
> > >  echo "bdev awu_min $bdev_min_write" >> $seqres.full
> > > -echo "bdev awu_min $bdev_max_write" >> $seqres.full
> > > +echo "bdev awu_max $bdev_max_write" >> $seqres.full
> > >  
> > >  # Test that statx atomic values are the same as sysfs values
> > >  if [ "$sys_min_write" -ne "$bdev_min_write" ]; then
> > > -- 
> > > 2.53.0
> > > 
> > > 
>