[PATCH 0/6] null_blk: allow REQ_OP_WRITE_ZEROES and cleanup

[PATCH 0/6] null_blk: allow REQ_OP_WRITE_ZEROES and cleanup

[Chaitanya Kulkarni]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 20908 bytes --]

Hi,

In order to test the non-trivial I/O path in the block layer for
REQ_OP_WRITE_ZEROES allow write-zeroes on null_blk so we can write
testcases with and without non-memory backed mode followed by
few cleanup patches.

Below is the test report with ext2/ext4 mkfs and a blktest
waiting to get upstream for this patch-series.

-ck

Chaitanya Kulkarni (6):
  null_blk: allow write zeores on non-membacked
  null_blk: allow write zeores on membacked
  null_blk: code cleaup
  null_blk: initialize cmd->bio in __alloc_cmd()
  null_blk: don't use magic numbers in the code
  null_blk: remove extra space in switch condition

 drivers/block/null_blk/main.c     | 131 ++++++++++++++++++++++--------
 drivers/block/null_blk/null_blk.h |   1 +
 2 files changed, 97 insertions(+), 35 deletions(-)

1. Write Zeroes test with ext2/ext3 for memory backed :-
linux-block (for-next) # mkfs.ext2 /dev/nullb0 
mke2fs 1.45.6 (20-Mar-2020)
Creating filesystem with 262144 4k blocks and 65536 inodes
Filesystem UUID: 76037c2c-9c32-4ac6-b1d6-d1f62e6071ad
Superblock backups stored on blocks: 
	32768, 98304, 163840, 229376

Allocating group tables: done                            
Writing inode tables: done                            
Writing superblocks and filesystem accounting information: done

linux-block (for-next) # mount /dev/nullb0 /mnt/
backend/ data/    nullb0/  nvme1n1/ test/    
linux-block (for-next) # mount /dev/nullb0 /mnt/nullb0/
linux-block (for-next) # dmesg  -c 
[18833.072830] null_blk: null_handle_write_zeroes 1225 2097024 128
[18833.072963] null_blk: null_handle_write_zeroes 1225 536 4096
[18833.073439] null_blk: null_handle_write_zeroes 1225 262680 4096
[18833.073845] null_blk: null_handle_write_zeroes 1225 524304 4096
[18833.074202] null_blk: null_handle_write_zeroes 1225 786968 4096
[18833.074581] null_blk: null_handle_write_zeroes 1225 1048592 4096
[18833.074951] null_blk: null_handle_write_zeroes 1225 1311256 4096
[18833.075381] null_blk: null_handle_write_zeroes 1225 1572880 4096
[18833.075748] null_blk: null_handle_write_zeroes 1225 1835544 4096
[18833.076164] null_blk: null_handle_write_zeroes 1225 4672 8
[18844.752019] EXT4-fs (nullb0): mounting ext2 file system using the ext4 subsystem
[18844.752487] EXT4-fs (nullb0): mounted filesystem without journal. Quota mode: none.
linux-block (for-next) # 


linux-block (for-next) # mkfs.ext4 /dev/nullb0 
mke2fs 1.45.6 (20-Mar-2020)
/dev/nullb0 contains a ext2 file system
	last mounted on Mon Oct  3 18:28:05 2022
Proceed anyway? (y,N) y
Creating filesystem with 262144 4k blocks and 65536 inodes
Filesystem UUID: a4c66d5f-bda9-4f58-aa03-911b87bb5c7d
Superblock backups stored on blocks: 
	32768, 98304, 163840, 229376

Allocating group tables: done                            
Writing inode tables: done                            
Creating journal (8192 blocks): done
Writing superblocks and filesystem accounting information: done

linux-block (for-next) # mount /dev/nullb0 /mnt/nullb0/
linux-block (for-next) # dmesg  -c 
[18882.721160] EXT4-fs (nullb0): unmounting filesystem.
[18888.687070] null_blk: null_handle_write_zeroes 1225 2097024 128
[18888.687159] null_blk: null_handle_write_zeroes 1225 1160 8
[18888.687309] null_blk: null_handle_write_zeroes 1225 33968 8
[18888.687839] null_blk: null_handle_write_zeroes 1225 1048576 65536
[18892.105964] EXT4-fs (nullb0): mounted filesystem with ordered data mode. Quota mode: none.
linux-block (for-next) # 

2. Test blkdiscard with -z for different block sizes :-

linux-block (for-next) # ./zeroout.sh 
#################### BLKISZ 512 #####################
config/nullb/nullb0
├── badblocks
├── blocking
├── blocksize
├── cache_size
├── completion_nsec
├── discard
├── home_node
├── hw_queue_depth
├── index
├── irqmode
├── max_sectors
├── mbps
├── memory_backed
├── no_sched
├── poll_queues
├── power
├── queue_mode
├── shared_tag_bitmap
├── size
├── submit_queues
├── use_per_node_hctx
├── virt_boundary
├── write_zeroes
├── zone_capacity
├── zoned
├── zone_max_active
├── zone_max_open
├── zone_nr_conv
└── zone_size

0 directories, 29 files
ODD:- 
19+1 records in
19+1 records out
10002 bytes (10 kB, 9.8 KiB) copied, 0.000521991 s, 19.2 MB/s
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0000512   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000528
0001024   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001040
0001536   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001552
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0002560   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002576
0003072   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003088
0003584   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003600
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0004608   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004624
------------------------------------------------------
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0000512  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0000528
0001024   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001040
0001536  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0001552
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0002560  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0002576
0003072   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003088
0003584  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0003600
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0004608  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0004624
------------------------------------------------------
EVEN:- 
19+1 records in
19+1 records out
10002 bytes (10 kB, 9.8 KiB) copied, 0.000376636 s, 26.6 MB/s
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0000512   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000528
0001024   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001040
0001536   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001552
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0002560   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002576
0003072   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003088
0003584   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003600
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0004608   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004624
------------------------------------------------------
0000000  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0000016
0000512   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000528
0001024  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0001040
0001536   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001552
0002048  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0002064
0002560   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002576
0003072  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0003088
0003584   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003600
0004096  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0004112
0004608   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004624
------------------------------------------------------




#################### BLKISZ 1024 #####################
config/nullb/nullb0
├── badblocks
├── blocking
├── blocksize
├── cache_size
├── completion_nsec
├── discard
├── home_node
├── hw_queue_depth
├── index
├── irqmode
├── max_sectors
├── mbps
├── memory_backed
├── no_sched
├── poll_queues
├── power
├── queue_mode
├── shared_tag_bitmap
├── size
├── submit_queues
├── use_per_node_hctx
├── virt_boundary
├── write_zeroes
├── zone_capacity
├── zoned
├── zone_max_active
├── zone_max_open
├── zone_nr_conv
└── zone_size

0 directories, 29 files
ODD:- 
9+1 records in
9+1 records out
10002 bytes (10 kB, 9.8 KiB) copied, 0.00040033 s, 25.0 MB/s
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0001024   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001040
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0003072   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003088
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0005120   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0005136
0006144   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0006160
0007168   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0007184
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0009216   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0009232
------------------------------------------------------
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0001024  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0001040
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0003072  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0003088
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0005120  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0005136
0006144   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0006160
0007168  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0007184
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0009216  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0009232
------------------------------------------------------
EVEN:- 
9+1 records in
9+1 records out
10002 bytes (10 kB, 9.8 KiB) copied, 0.000325488 s, 30.7 MB/s
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0001024   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001040
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0003072   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003088
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0005120   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0005136
0006144   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0006160
0007168   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0007184
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0009216   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0009232
------------------------------------------------------
0000000  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0000016
0001024   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0001040
0002048  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0002064
0003072   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0003088
0004096  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0004112
0005120   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0005136
0006144  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0006160
0007168   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0007184
0008192  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0008208
0009216   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0009232
------------------------------------------------------




#################### BLKISZ 2048 #####################
config/nullb/nullb0
├── badblocks
├── blocking
├── blocksize
├── cache_size
├── completion_nsec
├── discard
├── home_node
├── hw_queue_depth
├── index
├── irqmode
├── max_sectors
├── mbps
├── memory_backed
├── no_sched
├── poll_queues
├── power
├── queue_mode
├── shared_tag_bitmap
├── size
├── submit_queues
├── use_per_node_hctx
├── virt_boundary
├── write_zeroes
├── zone_capacity
├── zoned
├── zone_max_active
├── zone_max_open
├── zone_nr_conv
└── zone_size

0 directories, 29 files
ODD:- 
4+1 records in
4+1 records out
10002 bytes (10 kB, 9.8 KiB) copied, 0.000367949 s, 27.2 MB/s
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0006144   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0006160
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0010240  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0010256
0012288  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0012304
0014336  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0014352
0016384  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0016400
0018432  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0018448
------------------------------------------------------
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0002048  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0002064
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0006144  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0006160
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0010240  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0010256
0012288  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0012304
0014336  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0014352
0016384  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0016400
0018432  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0018448
------------------------------------------------------
EVEN:- 
4+1 records in
4+1 records out
10002 bytes (10 kB, 9.8 KiB) copied, 0.000310981 s, 32.2 MB/s
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0006144   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0006160
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0010240  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0010256
0012288  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0012304
0014336  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0014352
0016384  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0016400
0018432  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0018448
------------------------------------------------------
0000000  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0000016
0002048   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0002064
0004096  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0004112
0006144   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0006160
0008192  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0008208
0010240  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0010256
0012288  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0012304
0014336  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0014352
0016384  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0016400
0018432  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0018448
------------------------------------------------------




#################### BLKISZ 4096 #####################
config/nullb/nullb0
├── badblocks
├── blocking
├── blocksize
├── cache_size
├── completion_nsec
├── discard
├── home_node
├── hw_queue_depth
├── index
├── irqmode
├── max_sectors
├── mbps
├── memory_backed
├── no_sched
├── poll_queues
├── power
├── queue_mode
├── shared_tag_bitmap
├── size
├── submit_queues
├── use_per_node_hctx
├── virt_boundary
├── write_zeroes
├── zone_capacity
├── zoned
├── zone_max_active
├── zone_max_open
├── zone_nr_conv
└── zone_size

0 directories, 29 files
ODD:- 
2+1 records in
2+1 records out
10002 bytes (10 kB, 9.8 KiB) copied, 0.00028502 s, 35.1 MB/s
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0012288  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0012304
0016384  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0016400
0020480  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0020496
0024576  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0024592
0028672  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0028688
0032768  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0032784
0036864  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0036880
------------------------------------------------------
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0004096  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0004112
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0012288  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0012304
0016384  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0016400
0020480  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0020496
0024576  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0024592
0028672  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0028688
0032768  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0032784
0036864  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0036880
------------------------------------------------------
EVEN:- 
2+1 records in
2+1 records out
10002 bytes (10 kB, 9.8 KiB) copied, 0.00024765 s, 40.4 MB/s
0000000   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0000016
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0008192   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0008208
0012288  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0012304
0016384  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0016400
0020480  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0020496
0024576  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0024592
0028672  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0028688
0032768  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0032784
0036864  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0036880
------------------------------------------------------
0000000  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0000016
0004096   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a   a
0004112
0008192  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0008208
0012288  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0012304
0016384  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0016400
0020480  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0020496
0024576  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0024592
0028672  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0028688
0032768  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0032784
0036864  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
0036880
------------------------------------------------------

[PATCH 1/6] null_blk: allow write zeores on non-membacked

[Chaitanya Kulkarni]

Add a helper function to enable the REQ_OP_WRITE_ZEROES operations
when null_blk is configured with the non-membacked operations.

Since write-zeroes is a non-trivial I/O operation we need this to
add a blktest so we can test the non-trivial I/O path from the
application to the block layer.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/null_blk/main.c     | 13 +++++++++++++
 drivers/block/null_blk/null_blk.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 1f154f92f4c2..fc3e883f7b84 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -209,6 +209,10 @@ static bool g_discard;
 module_param_named(discard, g_discard, bool, 0444);
 MODULE_PARM_DESC(discard, "Support discard operations (requires memory-backed null_blk device). Default: false");
 
+static bool g_write_zeroes;
+module_param_named(write_zeroes, g_write_zeroes, bool, 0444);
+MODULE_PARM_DESC(write_zeroes, "Support write-zeores operations. Default: false");
+
 static unsigned long g_cache_size;
 module_param_named(cache_size, g_cache_size, ulong, 0444);
 MODULE_PARM_DESC(mbps, "Cache size in MiB for memory-backed device. Default: 0 (none)");
@@ -678,6 +682,7 @@ static struct nullb_device *null_alloc_dev(void)
 	dev->blocking = g_blocking;
 	dev->memory_backed = g_memory_backed;
 	dev->discard = g_discard;
+	dev->write_zeroes = g_write_zeroes;
 	dev->cache_size = g_cache_size;
 	dev->mbps = g_mbps;
 	dev->use_per_node_hctx = g_use_per_node_hctx;
@@ -1800,6 +1805,13 @@ static void null_config_discard(struct nullb *nullb)
 	blk_queue_max_discard_sectors(nullb->q, UINT_MAX >> 9);
 }
 
+static void null_config_write_zeroes(struct nullb *nullb)
+{
+	if (!nullb->dev->write_zeroes)
+		return;
+	blk_queue_max_write_zeroes_sectors(nullb->q, UINT_MAX >> 9);
+}
+
 static const struct block_device_operations null_bio_ops = {
 	.owner		= THIS_MODULE,
 	.submit_bio	= null_submit_bio,
@@ -2111,6 +2123,7 @@ static int null_add_dev(struct nullb_device *dev)
 		blk_queue_virt_boundary(nullb->q, PAGE_SIZE - 1);
 
 	null_config_discard(nullb);
+	null_config_write_zeroes(nullb);
 
 	if (config_item_name(&dev->item)) {
 		/* Use configfs dir name as the device name */
diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h
index 94ff68052b1e..2c0c9c29158f 100644
--- a/drivers/block/null_blk/null_blk.h
+++ b/drivers/block/null_blk/null_blk.h
@@ -111,6 +111,7 @@ struct nullb_device {
 	bool power; /* power on/off the device */
 	bool memory_backed; /* if data is stored in memory */
 	bool discard; /* if support discard */
+	bool write_zeroes; /* if support write_zeroes */
 	bool zoned; /* if device is zoned */
 	bool virt_boundary; /* virtual boundary on/off for the device */
 	bool no_sched; /* no IO scheduler for the device */
-- 
2.29.0

[PATCH 2/6] null_blk: allow write zeores on membacked

[Chaitanya Kulkarni]

Add a helper functions to enable the REQ_OP_WRITE_ZEROES operations
when null_blk is configured with the membacked mode.

Since write-zeroes is a non-trivial I/O operation we need this to
add a blktest so we can test the non-trivial I/O path from the
application to the block layer.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/null_blk/main.c | 46 ++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index fc3e883f7b84..2d592b4eb815 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -420,6 +420,7 @@ NULLB_DEVICE_ATTR(blocking, bool, NULL);
 NULLB_DEVICE_ATTR(use_per_node_hctx, bool, NULL);
 NULLB_DEVICE_ATTR(memory_backed, bool, NULL);
 NULLB_DEVICE_ATTR(discard, bool, NULL);
+NULLB_DEVICE_ATTR(write_zeroes, bool, NULL);
 NULLB_DEVICE_ATTR(mbps, uint, NULL);
 NULLB_DEVICE_ATTR(cache_size, ulong, NULL);
 NULLB_DEVICE_ATTR(zoned, bool, NULL);
@@ -544,6 +545,7 @@ static struct configfs_attribute *nullb_device_attrs[] = {
 	&nullb_device_attr_power,
 	&nullb_device_attr_memory_backed,
 	&nullb_device_attr_discard,
+	&nullb_device_attr_write_zeroes,
 	&nullb_device_attr_mbps,
 	&nullb_device_attr_cache_size,
 	&nullb_device_attr_badblocks,
@@ -618,7 +620,7 @@ static ssize_t memb_group_features_show(struct config_item *item, char *page)
 			"poll_queues,power,queue_mode,shared_tag_bitmap,size,"
 			"submit_queues,use_per_node_hctx,virt_boundary,zoned,"
 			"zone_capacity,zone_max_active,zone_max_open,"
-			"zone_nr_conv,zone_size\n");
+			"zone_nr_conv,zone_size,write_zeroes\n");
 }
 
 CONFIGFS_ATTR_RO(memb_group_, features);
@@ -875,6 +877,24 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
 	}
 }
 
+static void null_zero_sector(struct nullb_device *d, sector_t sect,
+			     sector_t nr_sects, bool cache)
+{
+	struct radix_tree_root *root = cache ? &d->cache : &d->data;
+	struct nullb_page *t_page;
+	unsigned int offset;
+	void *dest;
+
+	t_page = radix_tree_lookup(root, sect >> PAGE_SECTORS_SHIFT);
+	if (!t_page)
+		return;
+
+	offset = (sect & SECTOR_MASK) << SECTOR_SHIFT;
+	dest = kmap_atomic(t_page->page);
+	memset(dest + offset, 0, SECTOR_SIZE * nr_sects);
+	kunmap_atomic(dest);
+}
+
 static struct nullb_page *null_radix_tree_insert(struct nullb *nullb, u64 idx,
 	struct nullb_page *t_page, bool is_cache)
 {
@@ -1191,6 +1211,27 @@ blk_status_t null_handle_discard(struct nullb_device *dev,
 	return BLK_STS_OK;
 }
 
+static blk_status_t null_handle_write_zeroes(struct nullb_device *dev,
+					     sector_t sector, sector_t nr_sectors)
+{
+	unsigned int bytes_left = nr_sectors << 9;
+	struct nullb *nullb = dev->nullb;
+	size_t curr_bytes;
+
+	spin_lock_irq(&nullb->lock);
+	while (bytes_left > 0) {
+		curr_bytes = min_t(size_t, bytes_left, nullb->dev->blocksize);
+		nr_sectors = curr_bytes >> SECTOR_SHIFT;
+		null_zero_sector(nullb->dev, sector, nr_sectors, false);
+		if (null_cache_active(nullb))
+			null_zero_sector(nullb->dev, sector, nr_sectors, true);
+		sector += nr_sectors;
+		bytes_left -= curr_bytes;
+	}
+	spin_unlock_irq(&nullb->lock);
+	return BLK_STS_OK;
+}
+
 static int null_handle_flush(struct nullb *nullb)
 {
 	int err;
@@ -1357,6 +1398,9 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
 	if (op == REQ_OP_DISCARD)
 		return null_handle_discard(dev, sector, nr_sectors);
 
+	if (op == REQ_OP_WRITE_ZEROES)
+		return null_handle_write_zeroes(dev, sector, nr_sectors);
+
 	if (dev->queue_mode == NULL_Q_BIO)
 		err = null_handle_bio(cmd);
 	else
-- 
2.29.0

[PATCH 3/6] null_blk: code cleaup

[Chaitanya Kulkarni]

Introduce and use two new macros for calculating the page index from
given sector and index (offset) of the sector in the page.
The newly added macros makes code easy to read with meaningful name and
explanation comments attached to it.

While at it adjust the code in the null_free_sector() to return early
to get rid of the extra identation.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/null_blk/main.c | 37 ++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 2d592b4eb815..b82c2ffeb086 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -14,6 +14,11 @@
 #undef pr_fmt
 #define pr_fmt(fmt)	"null_blk: " fmt
 
+/* Gives page index for which this sector belongs to. */
+#define PAGE_IDX_FROM_SECT(sect)	(sect >> PAGE_SECTORS_SHIFT)
+/* Gives index (offset) of the sector within page. */
+#define SECT_IDX_IN_PAGE(sect)		((sect & SECTOR_MASK) << SECTOR_SHIFT)
+
 #define FREE_BATCH		16
 
 #define TICKS_PER_SEC		50ULL
@@ -860,20 +865,20 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
 	struct radix_tree_root *root;
 
 	root = is_cache ? &nullb->dev->cache : &nullb->dev->data;
-	idx = sector >> PAGE_SECTORS_SHIFT;
+	idx = PAGE_IDX_FROM_SECT(sector);
 	sector_bit = (sector & SECTOR_MASK);
 
 	t_page = radix_tree_lookup(root, idx);
-	if (t_page) {
-		__clear_bit(sector_bit, t_page->bitmap);
-
-		if (null_page_empty(t_page)) {
-			ret = radix_tree_delete_item(root, idx, t_page);
-			WARN_ON(ret != t_page);
-			null_free_page(ret);
-			if (is_cache)
-				nullb->dev->curr_cache -= PAGE_SIZE;
-		}
+	if (!t_page)
+		return;
+	__clear_bit(sector_bit, t_page->bitmap);
+
+	if (null_page_empty(t_page)) {
+		ret = radix_tree_delete_item(root, idx, t_page);
+		WARN_ON(ret != t_page);
+		null_free_page(ret);
+		if (is_cache)
+			nullb->dev->curr_cache -= PAGE_SIZE;
 	}
 }
 
@@ -885,11 +890,11 @@ static void null_zero_sector(struct nullb_device *d, sector_t sect,
 	unsigned int offset;
 	void *dest;
 
-	t_page = radix_tree_lookup(root, sect >> PAGE_SECTORS_SHIFT);
+	t_page = radix_tree_lookup(root, PAGE_IDX_FROM_SECT(sect));
 	if (!t_page)
 		return;
 
-	offset = (sect & SECTOR_MASK) << SECTOR_SHIFT;
+	offset = SECT_IDX_IN_PAGE(sect);
 	dest = kmap_atomic(t_page->page);
 	memset(dest + offset, 0, SECTOR_SIZE * nr_sects);
 	kunmap_atomic(dest);
@@ -949,7 +954,7 @@ static struct nullb_page *__null_lookup_page(struct nullb *nullb,
 	struct nullb_page *t_page;
 	struct radix_tree_root *root;
 
-	idx = sector >> PAGE_SECTORS_SHIFT;
+	idx = PAGE_IDX_FROM_SECT(sector);
 	sector_bit = (sector & SECTOR_MASK);
 
 	root = is_cache ? &nullb->dev->cache : &nullb->dev->data;
@@ -1125,7 +1130,7 @@ static int copy_to_nullb(struct nullb *nullb, struct page *source,
 		if (null_cache_active(nullb) && !is_fua)
 			null_make_cache_space(nullb, PAGE_SIZE);
 
-		offset = (sector & SECTOR_MASK) << SECTOR_SHIFT;
+		offset = SECT_IDX_IN_PAGE(sector);
 		t_page = null_insert_page(nullb, sector,
 			!null_cache_active(nullb) || is_fua);
 		if (!t_page)
@@ -1159,7 +1164,7 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest,
 	while (count < n) {
 		temp = min_t(size_t, nullb->dev->blocksize, n - count);
 
-		offset = (sector & SECTOR_MASK) << SECTOR_SHIFT;
+		offset = SECT_IDX_IN_PAGE(sector);
 		t_page = null_lookup_page(nullb, sector, false,
 			!null_cache_active(nullb));
 
-- 
2.29.0

[PATCH 4/6] null_blk: initialize cmd->bio in __alloc_cmd()

[Chaitanya Kulkarni]

The function __alloc_cmd() is responsible to allocate tag and
initializae the different members of the null_cmd structure e.g.
cmd->tag, cmd->error, and cmd->nq, Move only member bio that is initialized
from alloc_cmd() into __alloc_cmd().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/null_blk/main.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index b82c2ffeb086..765c1ca0edf5 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -743,7 +743,7 @@ static void free_cmd(struct nullb_cmd *cmd)
 
 static enum hrtimer_restart null_cmd_timer_expired(struct hrtimer *timer);
 
-static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq)
+static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq, struct bio *bio)
 {
 	struct nullb_cmd *cmd;
 	unsigned int tag;
@@ -754,6 +754,7 @@ static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq)
 		cmd->tag = tag;
 		cmd->error = BLK_STS_OK;
 		cmd->nq = nq;
+		cmd->bio = bio;
 		if (nq->dev->irqmode == NULL_IRQ_TIMER) {
 			hrtimer_init(&cmd->timer, CLOCK_MONOTONIC,
 				     HRTIMER_MODE_REL);
@@ -775,11 +776,9 @@ static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, struct bio *bio)
 		 * This avoids multiple return statements, multiple calls to
 		 * __alloc_cmd() and a fast path call to prepare_to_wait().
 		 */
-		cmd = __alloc_cmd(nq);
-		if (cmd) {
-			cmd->bio = bio;
+		cmd = __alloc_cmd(nq, bio);
+		if (cmd)
 			return cmd;
-		}
 		prepare_to_wait(&nq->wait, &wait, TASK_UNINTERRUPTIBLE);
 		io_schedule();
 		finish_wait(&nq->wait, &wait);
-- 
2.29.0

[PATCH 5/6] null_blk: don't use magic numbers in the code

[Chaitanya Kulkarni]

Insteasd of using the hardcoded value use meaningful macro for tag
available value of -1U in get_tag() and __alloc_cmd().

While at it return early on error to get rid of the extra indentation
in __alloc_cmd().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/null_blk/main.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 765c1ca0edf5..eda5050d6dee 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -24,6 +24,8 @@
 #define TICKS_PER_SEC		50ULL
 #define TIMER_INTERVAL		(NSEC_PER_SEC / TICKS_PER_SEC)
 
+#define NULL_REQ_TAG_NOT_AVAILABLE (-1U)
+
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
 static DECLARE_FAULT_ATTR(null_timeout_attr);
 static DECLARE_FAULT_ATTR(null_requeue_attr);
@@ -730,7 +732,7 @@ static unsigned int get_tag(struct nullb_queue *nq)
 	do {
 		tag = find_first_zero_bit(nq->tag_map, nq->queue_depth);
 		if (tag >= nq->queue_depth)
-			return -1U;
+			return NULL_REQ_TAG_NOT_AVAILABLE;
 	} while (test_and_set_bit_lock(tag, nq->tag_map));
 
 	return tag;
@@ -749,21 +751,19 @@ static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq, struct bio *bio)
 	unsigned int tag;
 
 	tag = get_tag(nq);
-	if (tag != -1U) {
-		cmd = &nq->cmds[tag];
-		cmd->tag = tag;
-		cmd->error = BLK_STS_OK;
-		cmd->nq = nq;
-		cmd->bio = bio;
-		if (nq->dev->irqmode == NULL_IRQ_TIMER) {
-			hrtimer_init(&cmd->timer, CLOCK_MONOTONIC,
-				     HRTIMER_MODE_REL);
-			cmd->timer.function = null_cmd_timer_expired;
-		}
-		return cmd;
+	if (tag == NULL_REQ_TAG_NOT_AVAILABLE)
+		return NULL;
+	cmd = &nq->cmds[tag];
+	cmd->tag = tag;
+	cmd->error = BLK_STS_OK;
+	cmd->nq = nq;
+	cmd->bio = bio;
+	if (nq->dev->irqmode == NULL_IRQ_TIMER) {
+		hrtimer_init(&cmd->timer, CLOCK_MONOTONIC,
+				HRTIMER_MODE_REL);
+		cmd->timer.function = null_cmd_timer_expired;
 	}
-
-	return NULL;
+	return cmd;
 }
 
 static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, struct bio *bio)
-- 
2.29.0

[PATCH 6/6] null_blk: remove extra space in switch condition

[Chaitanya Kulkarni]

The extra space in after switch condition does not follow kernel coding
standards, remove extra space in switch condition end_cmd().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/block/null_blk/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index eda5050d6dee..e030f87d0208 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -789,7 +789,7 @@ static void end_cmd(struct nullb_cmd *cmd)
 {
 	int queue_mode = cmd->nq->dev->queue_mode;
 
-	switch (queue_mode)  {
+	switch (queue_mode) {
 	case NULL_Q_MQ:
 		blk_mq_end_request(cmd->rq, cmd->error);
 		return;
-- 
2.29.0

Re: [PATCH 1/6] null_blk: allow write zeores on non-membacked

[Damien Le Moal]

On 10/5/22 12:16, Chaitanya Kulkarni wrote:
> Add a helper function to enable the REQ_OP_WRITE_ZEROES operations
> when null_blk is configured with the non-membacked operations.
> 
> Since write-zeroes is a non-trivial I/O operation we need this to
> add a blktest so we can test the non-trivial I/O path from the
> application to the block layer.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/null_blk/main.c     | 13 +++++++++++++
>  drivers/block/null_blk/null_blk.h |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 1f154f92f4c2..fc3e883f7b84 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -209,6 +209,10 @@ static bool g_discard;
>  module_param_named(discard, g_discard, bool, 0444);
>  MODULE_PARM_DESC(discard, "Support discard operations (requires memory-backed null_blk device). Default: false");
>  
> +static bool g_write_zeroes;
> +module_param_named(write_zeroes, g_write_zeroes, bool, 0444);
> +MODULE_PARM_DESC(write_zeroes, "Support write-zeores operations. Default: false");

Why not make this a number of sectors representing the maximum size of a
write zero command (blk_queue_max_write_zeroes_sectors()) ? That would
allow exercising split write zeros BIOs.

> +
>  static unsigned long g_cache_size;
>  module_param_named(cache_size, g_cache_size, ulong, 0444);
>  MODULE_PARM_DESC(mbps, "Cache size in MiB for memory-backed device. Default: 0 (none)");
> @@ -678,6 +682,7 @@ static struct nullb_device *null_alloc_dev(void)
>  	dev->blocking = g_blocking;
>  	dev->memory_backed = g_memory_backed;
>  	dev->discard = g_discard;
> +	dev->write_zeroes = g_write_zeroes;
>  	dev->cache_size = g_cache_size;
>  	dev->mbps = g_mbps;
>  	dev->use_per_node_hctx = g_use_per_node_hctx;
> @@ -1800,6 +1805,13 @@ static void null_config_discard(struct nullb *nullb)
>  	blk_queue_max_discard_sectors(nullb->q, UINT_MAX >> 9);
>  }
>  
> +static void null_config_write_zeroes(struct nullb *nullb)
> +{
> +	if (!nullb->dev->write_zeroes)
> +		return;
> +	blk_queue_max_write_zeroes_sectors(nullb->q, UINT_MAX >> 9);
> +}
> +
>  static const struct block_device_operations null_bio_ops = {
>  	.owner		= THIS_MODULE,
>  	.submit_bio	= null_submit_bio,
> @@ -2111,6 +2123,7 @@ static int null_add_dev(struct nullb_device *dev)
>  		blk_queue_virt_boundary(nullb->q, PAGE_SIZE - 1);
>  
>  	null_config_discard(nullb);
> +	null_config_write_zeroes(nullb);
>  
>  	if (config_item_name(&dev->item)) {
>  		/* Use configfs dir name as the device name */
> diff --git a/drivers/block/null_blk/null_blk.h b/drivers/block/null_blk/null_blk.h
> index 94ff68052b1e..2c0c9c29158f 100644
> --- a/drivers/block/null_blk/null_blk.h
> +++ b/drivers/block/null_blk/null_blk.h
> @@ -111,6 +111,7 @@ struct nullb_device {
>  	bool power; /* power on/off the device */
>  	bool memory_backed; /* if data is stored in memory */
>  	bool discard; /* if support discard */
> +	bool write_zeroes; /* if support write_zeroes */
>  	bool zoned; /* if device is zoned */
>  	bool virt_boundary; /* virtual boundary on/off for the device */
>  	bool no_sched; /* no IO scheduler for the device */

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/6] null_blk: allow write zeores on membacked

[Damien Le Moal]

On 10/5/22 12:16, Chaitanya Kulkarni wrote:
> Add a helper functions to enable the REQ_OP_WRITE_ZEROES operations
> when null_blk is configured with the membacked mode.
> 
> Since write-zeroes is a non-trivial I/O operation we need this to
> add a blktest so we can test the non-trivial I/O path from the
> application to the block layer.

Why a separate patch for this ? Introducing the module argument and
configfs equivalent separately in 2 different patches is strange and does
not facilitate review.

This patch should be merged with patch 1.

> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/null_blk/main.c | 46 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index fc3e883f7b84..2d592b4eb815 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -420,6 +420,7 @@ NULLB_DEVICE_ATTR(blocking, bool, NULL);
>  NULLB_DEVICE_ATTR(use_per_node_hctx, bool, NULL);
>  NULLB_DEVICE_ATTR(memory_backed, bool, NULL);
>  NULLB_DEVICE_ATTR(discard, bool, NULL);
> +NULLB_DEVICE_ATTR(write_zeroes, bool, NULL);
>  NULLB_DEVICE_ATTR(mbps, uint, NULL);
>  NULLB_DEVICE_ATTR(cache_size, ulong, NULL);
>  NULLB_DEVICE_ATTR(zoned, bool, NULL);
> @@ -544,6 +545,7 @@ static struct configfs_attribute *nullb_device_attrs[] = {
>  	&nullb_device_attr_power,
>  	&nullb_device_attr_memory_backed,
>  	&nullb_device_attr_discard,
> +	&nullb_device_attr_write_zeroes,
>  	&nullb_device_attr_mbps,
>  	&nullb_device_attr_cache_size,
>  	&nullb_device_attr_badblocks,
> @@ -618,7 +620,7 @@ static ssize_t memb_group_features_show(struct config_item *item, char *page)
>  			"poll_queues,power,queue_mode,shared_tag_bitmap,size,"
>  			"submit_queues,use_per_node_hctx,virt_boundary,zoned,"
>  			"zone_capacity,zone_max_active,zone_max_open,"
> -			"zone_nr_conv,zone_size\n");
> +			"zone_nr_conv,zone_size,write_zeroes\n");
>  }
>  
>  CONFIGFS_ATTR_RO(memb_group_, features);
> @@ -875,6 +877,24 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
>  	}
>  }
>  
> +static void null_zero_sector(struct nullb_device *d, sector_t sect,
> +			     sector_t nr_sects, bool cache)
> +{
> +	struct radix_tree_root *root = cache ? &d->cache : &d->data;
> +	struct nullb_page *t_page;
> +	unsigned int offset;
> +	void *dest;
> +
> +	t_page = radix_tree_lookup(root, sect >> PAGE_SECTORS_SHIFT);
> +	if (!t_page)
> +		return;
> +
> +	offset = (sect & SECTOR_MASK) << SECTOR_SHIFT;
> +	dest = kmap_atomic(t_page->page);
> +	memset(dest + offset, 0, SECTOR_SIZE * nr_sects);
> +	kunmap_atomic(dest);
> +}
> +
>  static struct nullb_page *null_radix_tree_insert(struct nullb *nullb, u64 idx,
>  	struct nullb_page *t_page, bool is_cache)
>  {
> @@ -1191,6 +1211,27 @@ blk_status_t null_handle_discard(struct nullb_device *dev,
>  	return BLK_STS_OK;
>  }
>  
> +static blk_status_t null_handle_write_zeroes(struct nullb_device *dev,
> +					     sector_t sector, sector_t nr_sectors)
> +{
> +	unsigned int bytes_left = nr_sectors << 9;
> +	struct nullb *nullb = dev->nullb;
> +	size_t curr_bytes;
> +
> +	spin_lock_irq(&nullb->lock);
> +	while (bytes_left > 0) {
> +		curr_bytes = min_t(size_t, bytes_left, nullb->dev->blocksize);
> +		nr_sectors = curr_bytes >> SECTOR_SHIFT;
> +		null_zero_sector(nullb->dev, sector, nr_sectors, false);
> +		if (null_cache_active(nullb))
> +			null_zero_sector(nullb->dev, sector, nr_sectors, true);
> +		sector += nr_sectors;
> +		bytes_left -= curr_bytes;
> +	}
> +	spin_unlock_irq(&nullb->lock);
> +	return BLK_STS_OK;
> +}
> +
>  static int null_handle_flush(struct nullb *nullb)
>  {
>  	int err;
> @@ -1357,6 +1398,9 @@ static inline blk_status_t null_handle_memory_backed(struct nullb_cmd *cmd,
>  	if (op == REQ_OP_DISCARD)
>  		return null_handle_discard(dev, sector, nr_sectors);
>  
> +	if (op == REQ_OP_WRITE_ZEROES)
> +		return null_handle_write_zeroes(dev, sector, nr_sectors);
> +
>  	if (dev->queue_mode == NULL_Q_BIO)
>  		err = null_handle_bio(cmd);
>  	else

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 3/6] null_blk: code cleaup

[Damien Le Moal]

On 10/5/22 12:16, Chaitanya Kulkarni wrote:
> Introduce and use two new macros for calculating the page index from
> given sector and index (offset) of the sector in the page.
> The newly added macros makes code easy to read with meaningful name and
> explanation comments attached to it.
> 
> While at it adjust the code in the null_free_sector() to return early
> to get rid of the extra identation.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/null_blk/main.c | 37 ++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 2d592b4eb815..b82c2ffeb086 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -14,6 +14,11 @@
>  #undef pr_fmt
>  #define pr_fmt(fmt)	"null_blk: " fmt
>  
> +/* Gives page index for which this sector belongs to. */

That is clear from the macro name. Not convinced this comment is useful.

> +#define PAGE_IDX_FROM_SECT(sect)	(sect >> PAGE_SECTORS_SHIFT)
> +/* Gives index (offset) of the sector within page. */
> +#define SECT_IDX_IN_PAGE(sect)		((sect & SECTOR_MASK) << SECTOR_SHIFT)

SECT_OFFSET_IN_PAGE() ? A "page" not being an array of sectors, using
index for a sector is a little strange I think. And you use this macro for
things like:

	offset = SECT_IDX_IN_PAGE(sect);

So offset in the name makes more sense.

> +
>  #define FREE_BATCH		16
>  
>  #define TICKS_PER_SEC		50ULL
> @@ -860,20 +865,20 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
>  	struct radix_tree_root *root;
>  
>  	root = is_cache ? &nullb->dev->cache : &nullb->dev->data;
> -	idx = sector >> PAGE_SECTORS_SHIFT;
> +	idx = PAGE_IDX_FROM_SECT(sector);
>  	sector_bit = (sector & SECTOR_MASK);
>  
>  	t_page = radix_tree_lookup(root, idx);
> -	if (t_page) {
> -		__clear_bit(sector_bit, t_page->bitmap);
> -
> -		if (null_page_empty(t_page)) {
> -			ret = radix_tree_delete_item(root, idx, t_page);
> -			WARN_ON(ret != t_page);
> -			null_free_page(ret);
> -			if (is_cache)
> -				nullb->dev->curr_cache -= PAGE_SIZE;
> -		}
> +	if (!t_page)
> +		return;
> +	__clear_bit(sector_bit, t_page->bitmap);
> +
> +	if (null_page_empty(t_page)) {
> +		ret = radix_tree_delete_item(root, idx, t_page);
> +		WARN_ON(ret != t_page);
> +		null_free_page(ret);
> +		if (is_cache)
> +			nullb->dev->curr_cache -= PAGE_SIZE;
>  	}
>  }
>  
> @@ -885,11 +890,11 @@ static void null_zero_sector(struct nullb_device *d, sector_t sect,
>  	unsigned int offset;
>  	void *dest;
>  
> -	t_page = radix_tree_lookup(root, sect >> PAGE_SECTORS_SHIFT);
> +	t_page = radix_tree_lookup(root, PAGE_IDX_FROM_SECT(sect));
>  	if (!t_page)
>  		return;
>  
> -	offset = (sect & SECTOR_MASK) << SECTOR_SHIFT;
> +	offset = SECT_IDX_IN_PAGE(sect);
>  	dest = kmap_atomic(t_page->page);
>  	memset(dest + offset, 0, SECTOR_SIZE * nr_sects);
>  	kunmap_atomic(dest);
> @@ -949,7 +954,7 @@ static struct nullb_page *__null_lookup_page(struct nullb *nullb,
>  	struct nullb_page *t_page;
>  	struct radix_tree_root *root;
>  
> -	idx = sector >> PAGE_SECTORS_SHIFT;
> +	idx = PAGE_IDX_FROM_SECT(sector);
>  	sector_bit = (sector & SECTOR_MASK);
>  
>  	root = is_cache ? &nullb->dev->cache : &nullb->dev->data;
> @@ -1125,7 +1130,7 @@ static int copy_to_nullb(struct nullb *nullb, struct page *source,
>  		if (null_cache_active(nullb) && !is_fua)
>  			null_make_cache_space(nullb, PAGE_SIZE);
>  
> -		offset = (sector & SECTOR_MASK) << SECTOR_SHIFT;
> +		offset = SECT_IDX_IN_PAGE(sector);
>  		t_page = null_insert_page(nullb, sector,
>  			!null_cache_active(nullb) || is_fua);
>  		if (!t_page)
> @@ -1159,7 +1164,7 @@ static int copy_from_nullb(struct nullb *nullb, struct page *dest,
>  	while (count < n) {
>  		temp = min_t(size_t, nullb->dev->blocksize, n - count);
>  
> -		offset = (sector & SECTOR_MASK) << SECTOR_SHIFT;
> +		offset = SECT_IDX_IN_PAGE(sector);
>  		t_page = null_lookup_page(nullb, sector, false,
>  			!null_cache_active(nullb));
>  

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/6] null_blk: initialize cmd->bio in __alloc_cmd()

[Damien Le Moal]

On 10/5/22 12:16, Chaitanya Kulkarni wrote:
> The function __alloc_cmd() is responsible to allocate tag and
> initializae the different members of the null_cmd structure e.g.
> cmd->tag, cmd->error, and cmd->nq, Move only member bio that is initialized
> from alloc_cmd() into __alloc_cmd().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/block/null_blk/main.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index b82c2ffeb086..765c1ca0edf5 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -743,7 +743,7 @@ static void free_cmd(struct nullb_cmd *cmd)
>  
>  static enum hrtimer_restart null_cmd_timer_expired(struct hrtimer *timer);
>  
> -static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq)
> +static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq, struct bio *bio)
>  {
>  	struct nullb_cmd *cmd;
>  	unsigned int tag;
> @@ -754,6 +754,7 @@ static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq)
>  		cmd->tag = tag;
>  		cmd->error = BLK_STS_OK;
>  		cmd->nq = nq;
> +		cmd->bio = bio;
>  		if (nq->dev->irqmode == NULL_IRQ_TIMER) {
>  			hrtimer_init(&cmd->timer, CLOCK_MONOTONIC,
>  				     HRTIMER_MODE_REL);
> @@ -775,11 +776,9 @@ static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, struct bio *bio)
>  		 * This avoids multiple return statements, multiple calls to
>  		 * __alloc_cmd() and a fast path call to prepare_to_wait().
>  		 */
> -		cmd = __alloc_cmd(nq);
> -		if (cmd) {
> -			cmd->bio = bio;
> +		cmd = __alloc_cmd(nq, bio);
> +		if (cmd)
>  			return cmd;
> -		}
>  		prepare_to_wait(&nq->wait, &wait, TASK_UNINTERRUPTIBLE);
>  		io_schedule();
>  		finish_wait(&nq->wait, &wait);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 5/6] null_blk: don't use magic numbers in the code

[Damien Le Moal]

On 10/5/22 12:17, Chaitanya Kulkarni wrote:
> Insteasd of using the hardcoded value use meaningful macro for tag
> available value of -1U in get_tag() and __alloc_cmd().
> 
> While at it return early on error to get rid of the extra indentation
> in __alloc_cmd().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/block/null_blk/main.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 765c1ca0edf5..eda5050d6dee 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -24,6 +24,8 @@
>  #define TICKS_PER_SEC		50ULL
>  #define TIMER_INTERVAL		(NSEC_PER_SEC / TICKS_PER_SEC)
>  
> +#define NULL_REQ_TAG_NOT_AVAILABLE (-1U)
> +
>  #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
>  static DECLARE_FAULT_ATTR(null_timeout_attr);
>  static DECLARE_FAULT_ATTR(null_requeue_attr);
> @@ -730,7 +732,7 @@ static unsigned int get_tag(struct nullb_queue *nq)
>  	do {
>  		tag = find_first_zero_bit(nq->tag_map, nq->queue_depth);
>  		if (tag >= nq->queue_depth)
> -			return -1U;
> +			return NULL_REQ_TAG_NOT_AVAILABLE;
>  	} while (test_and_set_bit_lock(tag, nq->tag_map));
>  
>  	return tag;
> @@ -749,21 +751,19 @@ static struct nullb_cmd *__alloc_cmd(struct nullb_queue *nq, struct bio *bio)
>  	unsigned int tag;
>  
>  	tag = get_tag(nq);
> -	if (tag != -1U) {
> -		cmd = &nq->cmds[tag];
> -		cmd->tag = tag;
> -		cmd->error = BLK_STS_OK;
> -		cmd->nq = nq;
> -		cmd->bio = bio;
> -		if (nq->dev->irqmode == NULL_IRQ_TIMER) {
> -			hrtimer_init(&cmd->timer, CLOCK_MONOTONIC,
> -				     HRTIMER_MODE_REL);
> -			cmd->timer.function = null_cmd_timer_expired;
> -		}
> -		return cmd;
> +	if (tag == NULL_REQ_TAG_NOT_AVAILABLE)
> +		return NULL;
> +	cmd = &nq->cmds[tag];
> +	cmd->tag = tag;
> +	cmd->error = BLK_STS_OK;
> +	cmd->nq = nq;
> +	cmd->bio = bio;
> +	if (nq->dev->irqmode == NULL_IRQ_TIMER) {
> +		hrtimer_init(&cmd->timer, CLOCK_MONOTONIC,
> +				HRTIMER_MODE_REL);
> +		cmd->timer.function = null_cmd_timer_expired;
>  	}
> -
> -	return NULL;
> +	return cmd;
>  }
>  
>  static struct nullb_cmd *alloc_cmd(struct nullb_queue *nq, struct bio *bio)

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 6/6] null_blk: remove extra space in switch condition

[Damien Le Moal]

On 10/5/22 12:17, Chaitanya Kulkarni wrote:
> The extra space in after switch condition does not follow kernel coding
> standards, remove extra space in switch condition end_cmd().
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/block/null_blk/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index eda5050d6dee..e030f87d0208 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -789,7 +789,7 @@ static void end_cmd(struct nullb_cmd *cmd)
>  {
>  	int queue_mode = cmd->nq->dev->queue_mode;
>  
> -	switch (queue_mode)  {
> +	switch (queue_mode) {
>  	case NULL_Q_MQ:
>  		blk_mq_end_request(cmd->rq, cmd->error);
>  		return;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/6] null_blk: allow write zeores on membacked

[Chaitanya Kulkarni]

On 10/4/22 21:57, Damien Le Moal wrote:
> On 10/5/22 12:16, Chaitanya Kulkarni wrote:
>> Add a helper functions to enable the REQ_OP_WRITE_ZEROES operations
>> when null_blk is configured with the membacked mode.
>>
>> Since write-zeroes is a non-trivial I/O operation we need this to
>> add a blktest so we can test the non-trivial I/O path from the
>> application to the block layer.
> 
> Why a separate patch for this ? Introducing the module argument and
> configfs equivalent separately in 2 different patches is strange and does
> not facilitate review.
> 
> This patch should be merged with patch 1.
> 

Sounds good will merge it.

-ck

Re: [PATCH 3/6] null_blk: code cleaup

[Chaitanya Kulkarni]

On 10/4/22 22:02, Damien Le Moal wrote:
> On 10/5/22 12:16, Chaitanya Kulkarni wrote:
>> Introduce and use two new macros for calculating the page index from
>> given sector and index (offset) of the sector in the page.
>> The newly added macros makes code easy to read with meaningful name and
>> explanation comments attached to it.
>>
>> While at it adjust the code in the null_free_sector() to return early
>> to get rid of the extra identation.
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/block/null_blk/main.c | 37 ++++++++++++++++++++---------------
>>   1 file changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>> index 2d592b4eb815..b82c2ffeb086 100644
>> --- a/drivers/block/null_blk/main.c
>> +++ b/drivers/block/null_blk/main.c
>> @@ -14,6 +14,11 @@
>>   #undef pr_fmt
>>   #define pr_fmt(fmt)	"null_blk: " fmt
>>   
>> +/* Gives page index for which this sector belongs to. */
> 
> That is clear from the macro name. Not convinced this comment is useful.
> 
>> +#define PAGE_IDX_FROM_SECT(sect)	(sect >> PAGE_SECTORS_SHIFT)
>> +/* Gives index (offset) of the sector within page. */
>> +#define SECT_IDX_IN_PAGE(sect)		((sect & SECTOR_MASK) << SECTOR_SHIFT)
> 
> SECT_OFFSET_IN_PAGE() ? A "page" not being an array of sectors, using
> index for a sector is a little strange I think. And you use this macro for
> things like:
> 
> 	offset = SECT_IDX_IN_PAGE(sect);
> 
> So offset in the name makes more sense.
> 
>

okay I'll make it to SECT_OFFSET_IN_PAGE()

-ck

Re: [PATCH 1/6] null_blk: allow write zeores on non-membacked

[Chaitanya Kulkarni]

On 10/4/22 21:54, Damien Le Moal wrote:
> On 10/5/22 12:16, Chaitanya Kulkarni wrote:
>> Add a helper function to enable the REQ_OP_WRITE_ZEROES operations
>> when null_blk is configured with the non-membacked operations.
>>
>> Since write-zeroes is a non-trivial I/O operation we need this to
>> add a blktest so we can test the non-trivial I/O path from the
>> application to the block layer.
>>
>> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/block/null_blk/main.c     | 13 +++++++++++++
>>   drivers/block/null_blk/null_blk.h |  1 +
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
>> index 1f154f92f4c2..fc3e883f7b84 100644
>> --- a/drivers/block/null_blk/main.c
>> +++ b/drivers/block/null_blk/main.c
>> @@ -209,6 +209,10 @@ static bool g_discard;
>>   module_param_named(discard, g_discard, bool, 0444);
>>   MODULE_PARM_DESC(discard, "Support discard operations (requires memory-backed null_blk device). Default: false");
>>   
>> +static bool g_write_zeroes;
>> +module_param_named(write_zeroes, g_write_zeroes, bool, 0444);
>> +MODULE_PARM_DESC(write_zeroes, "Support write-zeores operations. Default: false");
> 
> Why not make this a number of sectors representing the maximum size of a
> write zero command (blk_queue_max_write_zeroes_sectors()) ? That would
> allow exercising split write zeros BIOs.
> 

I kept the implementation identical to the g_discard.

Perhaps it's time to change it so REQ_OP_DISCARD and
REQ_OP_WRITE_ZEROES will have same implementation.

I'll add a discard patch to match your suggested write-zeroes
behavior.

-ck

Re: [PATCH 2/6] null_blk: allow write zeores on membacked

[Brian Foster]

On Tue, Oct 04, 2022 at 08:16:57PM -0700, Chaitanya Kulkarni wrote:
> Add a helper functions to enable the REQ_OP_WRITE_ZEROES operations
> when null_blk is configured with the membacked mode.
> 
> Since write-zeroes is a non-trivial I/O operation we need this to
> add a blktest so we can test the non-trivial I/O path from the
> application to the block layer.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/block/null_blk/main.c | 46 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index fc3e883f7b84..2d592b4eb815 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
...
> @@ -875,6 +877,24 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
>  	}
>  }
>  
> +static void null_zero_sector(struct nullb_device *d, sector_t sect,
> +			     sector_t nr_sects, bool cache)
> +{

Any reason to not just pass the tree root directly here instead of the
cache boolean? It might make the callers more readable and also
eliminates the need to pass the nullb_device.

Brian

> +	struct radix_tree_root *root = cache ? &d->cache : &d->data;
> +	struct nullb_page *t_page;
> +	unsigned int offset;
> +	void *dest;
> +
> +	t_page = radix_tree_lookup(root, sect >> PAGE_SECTORS_SHIFT);
> +	if (!t_page)
> +		return;
> +
> +	offset = (sect & SECTOR_MASK) << SECTOR_SHIFT;
> +	dest = kmap_atomic(t_page->page);
> +	memset(dest + offset, 0, SECTOR_SIZE * nr_sects);
> +	kunmap_atomic(dest);
> +}
> +
>  static struct nullb_page *null_radix_tree_insert(struct nullb *nullb, u64 idx,
>  	struct nullb_page *t_page, bool is_cache)
>  {
...

Re: [PATCH 1/6] null_blk: allow write zeores on non-membacked

[Chaitanya Kulkarni]

>>> +static bool g_write_zeroes;
>>> +module_param_named(write_zeroes, g_write_zeroes, bool, 0444);
>>> +MODULE_PARM_DESC(write_zeroes, "Support write-zeores operations. Default: false");
>>
>> Why not make this a number of sectors representing the maximum size of a
>> write zero command (blk_queue_max_write_zeroes_sectors()) ? That would
>> allow exercising split write zeros BIOs.
>>
> 
> I kept the implementation identical to the g_discard.
> 
> Perhaps it's time to change it so REQ_OP_DISCARD and
> REQ_OP_WRITE_ZEROES will have same implementation.
> 
> I'll add a discard patch to match your suggested write-zeroes
> behavior.
> 
> -ck
> 

REQ_OP_DISCARD and REQ_OP_WRITE_ZEORES needs to be consistent
when it comes to configuration interface.

I did the change you suggested, it is breaking the backward
compatibility of discard and if we do it only for write-zeroes it
will be inconsistent with the current g_discard behavior.

Let's keep the original behavior and not break the backward
compatibility ?

-ck

Re: [PATCH 2/6] null_blk: allow write zeores on membacked

[Chaitanya Kulkarni]

>> @@ -875,6 +877,24 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
>>   	}
>>   }
>>   
>> +static void null_zero_sector(struct nullb_device *d, sector_t sect,
>> +			     sector_t nr_sects, bool cache)
>> +{
> 
> Any reason to not just pass the tree root directly here instead of the
> cache boolean? It might make the callers more readable and also
> eliminates the need to pass the nullb_device.
> 
> Brian
> 

I kept the style similar to null_free_sector() where root is calculated 
inside the helper acting on the sector, if we change that for 
null_zero_sector() then I think we need to change for null_free_sector() 
for consistency, unless you strongly object it for some reason.

-ck

Re: [PATCH 2/6] null_blk: allow write zeores on membacked

[Brian Foster]

On Wed, Oct 05, 2022 at 06:45:35PM +0000, Chaitanya Kulkarni wrote:
> 
> >> @@ -875,6 +877,24 @@ static void null_free_sector(struct nullb *nullb, sector_t sector,
> >>   	}
> >>   }
> >>   
> >> +static void null_zero_sector(struct nullb_device *d, sector_t sect,
> >> +			     sector_t nr_sects, bool cache)
> >> +{
> > 
> > Any reason to not just pass the tree root directly here instead of the
> > cache boolean? It might make the callers more readable and also
> > eliminates the need to pass the nullb_device.
> > 
> > Brian
> > 
> 
> I kept the style similar to null_free_sector() where root is calculated 
> inside the helper acting on the sector, if we change that for 
> null_zero_sector() then I think we need to change for null_free_sector() 
> for consistency, unless you strongly object it for some reason.
> 

Nope, just a nit that stood out to me when skimming the patches. Not a
big deal at all.

Brian

> -ck
> 

Re: [PATCH 2/6] null_blk: allow write zeores on membacked

[Chaitanya Kulkarni]

>>
>> I kept the style similar to null_free_sector() where root is calculated
>> inside the helper acting on the sector, if we change that for
>> null_zero_sector() then I think we need to change for null_free_sector()
>> for consistency, unless you strongly object it for some reason.
>>
> 
> Nope, just a nit that stood out to me when skimming the patches. Not a
> big deal at all.
> 

It is a really good point, I'll send a cleanup on the top of this will 
CC you.

-ck