Re: [PATCH for-next v3 3/5] null_blk: fix zone resource management for badblocks

On Jan 18, 2025 / 08:00, Damien Le Moal wrote:

On 1/15/25 1:29 PM, Shin'ichiro Kawasaki wrote:

quoted

When the badblocks parameter is set for zoned null_blk, zone resource
management does not work correctly. This issue arises because
null_zone_write() modifies the zone resource status and then call
null_process_cmd(), which handles the badblocks parameter. When
badblocks cause IO failures and no IO happens, the zone resource status
should not change. However, it has already changed.

And that is correct in general. E.g. if an SMR HDD encounters a bad block while
executing a write command, the bad block may endup failing the write command but
the write operation was already started, meaning that the zone was at least
implicitly open first to start writing. So doing zone management first and then
handling the bad blocks (eventually failing the write operation) is the correct
order to do things.

I commented on the previous version that partially advancing the wp for a write
that is failed due to a bad block was incorrect because zone resource management
needed to be done. But it seems I was mistaken since you are saying here that
zone management is already done before handling bad blocks. So I do not think we
need this change. Or is it me who is completely confused ?

Based on your comment on the previous version, I checked the code and found
the call chain was as follows:

null_process_zoned_cmd()
 null_zone_write()
  do zone resource management: zone resource management is done here,
                               assuming writes always succeed
  null_process_cmd()
   null_handle_badblocks() ... v2 3rd patch added wp move for partial writes

So, the zone resource management was done before applying the v2 patch series.

However, the zone resource management did not care the write failure by
badblocks. It assumed that as if the writes fully succeed always. When badblocks
causes write failure for zoned null_blk, it leaves wrong zone resource status.

I would say, this patch does not respond to your comment for the previous
version. It addresses the problem I found when I thought about your comment.

With this patch, the function call chain will be as follows:

null_process_zoned_cmd()
 null_zone_write()
  null_handle_badblocks()     ... checks how many sectors to be written
  do zone resource management ... zone resource management is done reflecting
                                  the result of null_handle_badblocks() call
   null_handle_memory_backed()

The zone resource management part will be skipped when badblocks causes no
write. The 5th patch in this 3rd series will modify null_handle_badblocks() to
support partial writes by badblocks, and the zone resource management will be
done for such partial writes.