| Message ID | 20241119002815.600608-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Improve write performance for zoned UFS devices | expand |
On 11/19/24 9:27 AM, Bart Van Assche wrote: > Fix a reference count leak in disk_zone_wplug_handle_error() > > Fixes: dd291d77cc90 ("block: Introduce zone write plugging") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > block/blk-zoned.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/block/blk-zoned.c b/block/blk-zoned.c > index 70211751df16..3346b8c53605 100644 > --- a/block/blk-zoned.c > +++ b/block/blk-zoned.c > @@ -1337,6 +1337,8 @@ static void disk_zone_wplug_handle_error(struct gendisk *disk, > > unlock: > spin_unlock_irqrestore(&zwplug->lock, flags); > + > + disk_put_zone_wplug(zwplug); The zone wplug put call is right after the single call site to disk_zone_wplug_handle_error(). The reason it is *not* in that function is that the reference on the wplug for handling an error is taken when the wplug is added to the error list. disk_zone_wplug_handle_error() does not itself take a reference on the wplug. So how did you come up with this ? What workload/operation did you run to find an issue ? > } > > static void disk_zone_wplugs_work(struct work_struct *work)
On 11/18/24 6:23 PM, Damien Le Moal wrote: > On 11/19/24 9:27 AM, Bart Van Assche wrote: >> Fix a reference count leak in disk_zone_wplug_handle_error() >> >> Fixes: dd291d77cc90 ("block: Introduce zone write plugging") >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> --- >> block/blk-zoned.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/block/blk-zoned.c b/block/blk-zoned.c >> index 70211751df16..3346b8c53605 100644 >> --- a/block/blk-zoned.c >> +++ b/block/blk-zoned.c >> @@ -1337,6 +1337,8 @@ static void disk_zone_wplug_handle_error(struct gendisk *disk, >> >> unlock: >> spin_unlock_irqrestore(&zwplug->lock, flags); >> + >> + disk_put_zone_wplug(zwplug); > > The zone wplug put call is right after the single call site to > disk_zone_wplug_handle_error(). The reason it is *not* in that function is that > the reference on the wplug for handling an error is taken when the wplug is > added to the error list. disk_zone_wplug_handle_error() does not itself take a > reference on the wplug. > > So how did you come up with this ? What workload/operation did you run to find > an issue ? Thanks for the feedback. I can't reproduce the refcount leak without my other patches. I will check with which other patch this patch has to be combined. This is the script that triggered the leak: #!/bin/bash set -eu qd=${1:-64} if modprobe -r scsi_debug; then :; fi params=( delay=0 dev_size_mb=256 every_nth=$((2 * qd)) max_queue=${qd} ndelay=100000 # 100 us opts=0x8000 # SDEBUG_OPT_HOST_BUSY preserves_write_order=1 sector_size=4096 zbc=host-managed zone_nr_conv=0 zone_size_mb=4 ) modprobe scsi_debug "${params[@]}" while true; do bdev=$(cd /sys/bus/pseudo/drivers/scsi_debug/adapter*/host*/target*/*/block && echo *) 2>/dev/null if [ -e /dev/"${bdev}" ]; then break; fi sleep .1 done dev=/dev/"${bdev}" [ -b "${dev}" ] for rw in write randwrite; do params=( --direct=1 --filename="${dev}" --iodepth="${qd}" --ioengine=io_uring --ioscheduler=none --gtod_reduce=1 --hipri=0 --name="$(basename "${dev}")" --runtime=30 --rw="$rw" --time_based=1 --zonemode=zbd ) fio "${params[@]}" rc=$? if grep -avH " ref 1 " /sys/kernel/debug/block/sda/zone_wplugs; then :; fi echo '' [ $rc = 0 ] || break done
diff --git a/block/blk-zoned.c b/block/blk-zoned.c index 70211751df16..3346b8c53605 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -1337,6 +1337,8 @@ static void disk_zone_wplug_handle_error(struct gendisk *disk, unlock: spin_unlock_irqrestore(&zwplug->lock, flags); + + disk_put_zone_wplug(zwplug); } static void disk_zone_wplugs_work(struct work_struct *work)
Fix a reference count leak in disk_zone_wplug_handle_error() Fixes: dd291d77cc90 ("block: Introduce zone write plugging") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- block/blk-zoned.c | 2 ++ 1 file changed, 2 insertions(+)