Hi Mikulas,
On Thu, Aug 10, 2023 at 12:07:07PM +0200, Mikulas Patocka wrote:
> Hi
>
> Here I'm submitting the ramdisk discard patches for the next merge window.
> If you want to make some more changes, please let me now.
brd discard is removed in f09a06a193d9 ("brd: remove discard support")
in 2017 because it is just driver private write_zero, and user can get same
result with fallocate(FALLOC_FL_ZERO_RANGE).
Also you only mentioned the motivation in V1 cover-letter:
https://lore.kernel.org/linux-block/alpine.LRH.2.02.2209151604410.13231@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
```
Zdenek asked me to write it, because we use brd in the lvm2 testsuite and
it would be benefical to run the testsuite with discard enabled in order
to test discard handling.
```
But we have lots of test disks with discard support: loop, scsi_debug,
null_blk, ublk, ..., so one requestion is that why brd discard is
a must for lvm2 testsuite to cover (lvm)discard handling?
The reason why brd didn't support discard by freeing pages is writeback
deadlock risk, see:
commit f09a06a193d9 ("brd: remove discard support")
-static void discard_from_brd(struct brd_device *brd,
- sector_t sector, size_t n)
-{
- while (n >= PAGE_SIZE) {
- /*
- * Don't want to actually discard pages here because
- * re-allocating the pages can result in writeback
- * deadlocks under heavy load.
- */
- if (0)
- brd_free_page(brd, sector);
- else
- brd_zero_page(brd, sector);
- sector += PAGE_SIZE >> SECTOR_SHIFT;
- n -= PAGE_SIZE;
- }
-}
However, you didn't mention how your patches address this potential
risk, care to document it? I can't find any related words about
this problem.
BTW, your patches looks more complicated than the original removed
discard implementation. And if the above questions get addressed,
I am happy to provide review on the following patches.
Thanks,
Ming
