On 11/3/25 16:18, Daniel Vacek wrote: > On Mon, 3 Nov 2025 at 06:55, Damien Le Moal <[email protected]> wrote: >> >> On 11/1/25 02:48, Bart Van Assche wrote: >>> Hi Damien, >>> >>> disk_update_zone_resources() only has a single caller and just below the >>> only call of this function the following code is present: >>> >>> if (ret) { >>> unsigned int memflags = blk_mq_freeze_queue(q); >>> >>> disk_free_zone_resources(disk); >>> blk_mq_unfreeze_queue(q, memflags); >>> } >>> >>> Shouldn't this code be moved into disk_update_zone_resources() such that >>> error handling happens without unfreezing and refreezing the request >>> queue? >> >> Check the code again. disk_free_zone_resources() if the report zones >> callbacks >> return an error, and in that case disk_update_zone_resources() is not called. >> So having this call as it is cover all cases. > > I understand Bart's idea was more like below: > >> @@ -1568,7 +1572,12 @@ static int disk_update_zone_resources(str > uct gendisk *disk, >> } >> >> commit: >> - return queue_limits_commit_update_frozen(q, &lim); >> + ret = queue_limits_commit_update(q, &lim); >> + >> +unfreeze: > > + if (ret) > + disk_free_zone_resources(disk); > >> + blk_mq_unfreeze_queue(q, memflags); >> + >> + return ret; >> } >> >> static int blk_revalidate_conv_zone(struct blk_zone *zone, unsigned int >> idx, > > And then in blk_revalidate_disk_zones() do this: > > if (ret > 0) { > ret = disk_update_zone_resources(disk, &args); > } else if (ret) { > unsigned int memflags; > > pr_warn("%s: failed to revalidate zones\n", disk->disk_name); > > memflags = blk_mq_freeze_queue(q); > disk_free_zone_resources(disk); > blk_mq_unfreeze_queue(q, memflags); > } > > The question remains if this looks better?
Rereading everything, I think that Bart has a good point: moving the call to disk_free_zone_resources() in the error path of disk_update_zone_resources() allows doing the error handling without unfreezing and re-freezing the queue. That's better. -- Damien Le Moal Western Digital Research
