diff mbox series

[3/4] btrfs: zoned: fail mount if the device does not support zone append

Message ID 20210416030528.757513-4-damien.lemoal@wdc.com (mailing list archive)
State New, archived
Headers show
Series Fix dm-crypt zoned block device support | expand

Commit Message

Damien Le Moal April 16, 2021, 3:05 a.m. UTC
From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

For zoned btrfs, zone append is mandatory to write to a sequential write
only zone, otherwise parallel writes to the same zone could result in
unaligned write errors.

If a zoned block device does not support zone append (e.g. a dm-crypt
zoned device using a non-NULL IV cypher), fail to mount.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 fs/btrfs/zoned.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

David Sterba April 16, 2021, 4:17 p.m. UTC | #1
On Fri, Apr 16, 2021 at 12:05:27PM +0900, Damien Le Moal wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> For zoned btrfs, zone append is mandatory to write to a sequential write
> only zone, otherwise parallel writes to the same zone could result in
> unaligned write errors.
> 
> If a zoned block device does not support zone append (e.g. a dm-crypt
> zoned device using a non-NULL IV cypher), fail to mount.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>

Added to misc-next, thanks. I'll queue it for 5.13, it's not an urgent
fix for 5.12 release but i'll tag it as stable so it'll apear in 5.12.x
later.
Christoph Hellwig April 19, 2021, 9:28 a.m. UTC | #2
On Fri, Apr 16, 2021 at 06:17:21PM +0200, David Sterba wrote:
> On Fri, Apr 16, 2021 at 12:05:27PM +0900, Damien Le Moal wrote:
> > From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > 
> > For zoned btrfs, zone append is mandatory to write to a sequential write
> > only zone, otherwise parallel writes to the same zone could result in
> > unaligned write errors.
> > 
> > If a zoned block device does not support zone append (e.g. a dm-crypt
> > zoned device using a non-NULL IV cypher), fail to mount.
> > 
> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> 
> Added to misc-next, thanks. I'll queue it for 5.13, it's not an urgent
> fix for 5.12 release but i'll tag it as stable so it'll apear in 5.12.x
> later.

Please don't.  Zone append is a strict requirement for zoned devices,
no need to add cargo cult code like this everywhere.
Damien Le Moal April 19, 2021, 9:35 a.m. UTC | #3
On 2021/04/19 18:29, Christoph Hellwig wrote:
> On Fri, Apr 16, 2021 at 06:17:21PM +0200, David Sterba wrote:
>> On Fri, Apr 16, 2021 at 12:05:27PM +0900, Damien Le Moal wrote:
>>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>>
>>> For zoned btrfs, zone append is mandatory to write to a sequential write
>>> only zone, otherwise parallel writes to the same zone could result in
>>> unaligned write errors.
>>>
>>> If a zoned block device does not support zone append (e.g. a dm-crypt
>>> zoned device using a non-NULL IV cypher), fail to mount.
>>>
>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>
>> Added to misc-next, thanks. I'll queue it for 5.13, it's not an urgent
>> fix for 5.12 release but i'll tag it as stable so it'll apear in 5.12.x
>> later.
> 
> Please don't.  Zone append is a strict requirement for zoned devices,
> no need to add cargo cult code like this everywhere.

This is only to avoid someone from running zoned-btrfs on top of dm-crypt.
Without this patch, mount will be OK and file data writes will also actually be
OK. But all reads will miserably fail... I would rather have this patch in than
deal with the "bug reports" about btrfs failing to read files. No ?

Note that like you, I dislike having to add such code. But it was my oversight
when I worked on getting dm-crypt to work on zoned drives. Zone append was
overlooked at that time... My bad, really.
Christoph Hellwig April 19, 2021, 9:39 a.m. UTC | #4
On Mon, Apr 19, 2021 at 09:35:37AM +0000, Damien Le Moal wrote:
> This is only to avoid someone from running zoned-btrfs on top of dm-crypt.
> Without this patch, mount will be OK and file data writes will also actually be
> OK. But all reads will miserably fail... I would rather have this patch in than
> deal with the "bug reports" about btrfs failing to read files. No ?
> 
> Note that like you, I dislike having to add such code. But it was my oversight
> when I worked on getting dm-crypt to work on zoned drives. Zone append was
> overlooked at that time... My bad, really.

dm-crypt needs to stop pretending it supports zoned devices if it
doesn't.  Note that dm-crypt could fairly trivially support zone append
by doing the same kind of emulation that the sd driver does.
Johannes Thumshirn April 19, 2021, 9:39 a.m. UTC | #5
On 19/04/2021 11:29, Christoph Hellwig wrote:
> On Fri, Apr 16, 2021 at 06:17:21PM +0200, David Sterba wrote:
>> On Fri, Apr 16, 2021 at 12:05:27PM +0900, Damien Le Moal wrote:
>>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>>
>>> For zoned btrfs, zone append is mandatory to write to a sequential write
>>> only zone, otherwise parallel writes to the same zone could result in
>>> unaligned write errors.
>>>
>>> If a zoned block device does not support zone append (e.g. a dm-crypt
>>> zoned device using a non-NULL IV cypher), fail to mount.
>>>
>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>
>> Added to misc-next, thanks. I'll queue it for 5.13, it's not an urgent
>> fix for 5.12 release but i'll tag it as stable so it'll apear in 5.12.x
>> later.
> 
> Please don't.  Zone append is a strict requirement for zoned devices,
> no need to add cargo cult code like this everywhere.
> 

As of now, dm-crypt supports zoned devices but cannot really work with 
zone append. At least not with ciphers that use sectors as IV.
OTOH btrfs cannot work without zone append. While we won't notice any 
problems with writes, reads (or better decrypt) will fail.
Damien Le Moal April 19, 2021, 9:46 a.m. UTC | #6
On 2021/04/19 18:41, hch@infradead.org wrote:
> On Mon, Apr 19, 2021 at 09:35:37AM +0000, Damien Le Moal wrote:
>> This is only to avoid someone from running zoned-btrfs on top of dm-crypt.
>> Without this patch, mount will be OK and file data writes will also actually be
>> OK. But all reads will miserably fail... I would rather have this patch in than
>> deal with the "bug reports" about btrfs failing to read files. No ?
>>
>> Note that like you, I dislike having to add such code. But it was my oversight
>> when I worked on getting dm-crypt to work on zoned drives. Zone append was
>> overlooked at that time... My bad, really.
> 
> dm-crypt needs to stop pretending it supports zoned devices if it
> doesn't.  Note that dm-crypt could fairly trivially support zone append
> by doing the same kind of emulation that the sd driver does.

I am not so sure about the "trivial" but yes, it is feasible. Let me think about
something then. Whatever we do, performance with ZNS will no be great, for
sure... But for SMR HDDs, we likely will not notice any difference in performance.
David Sterba April 19, 2021, 4:48 p.m. UTC | #7
On Mon, Apr 19, 2021 at 09:46:36AM +0000, Damien Le Moal wrote:
> On 2021/04/19 18:41, hch@infradead.org wrote:
> > On Mon, Apr 19, 2021 at 09:35:37AM +0000, Damien Le Moal wrote:
> >> This is only to avoid someone from running zoned-btrfs on top of dm-crypt.
> >> Without this patch, mount will be OK and file data writes will also actually be
> >> OK. But all reads will miserably fail... I would rather have this patch in than
> >> deal with the "bug reports" about btrfs failing to read files. No ?
> >>
> >> Note that like you, I dislike having to add such code. But it was my oversight
> >> when I worked on getting dm-crypt to work on zoned drives. Zone append was
> >> overlooked at that time... My bad, really.
> > 
> > dm-crypt needs to stop pretending it supports zoned devices if it
> > doesn't.  Note that dm-crypt could fairly trivially support zone append
> > by doing the same kind of emulation that the sd driver does.
> 
> I am not so sure about the "trivial" but yes, it is feasible. Let me think about
> something then. Whatever we do, performance with ZNS will no be great, for
> sure... But for SMR HDDs, we likely will not notice any difference in performance.

So this needs to be fixed outside of btrfs. The fix in btrfs would make
sense in case we can't sync the dm-crypt and btrfs in a released kernel.
Having a mount check sounds like a better option to me than to fail
reads, we can revert it in a release once everything woks as expected.
diff mbox series

Patch

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index eeb3ebe11d7a..70b23a0d03b1 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -342,6 +342,13 @@  int btrfs_get_dev_zone_info(struct btrfs_device *device)
 	if (!IS_ALIGNED(nr_sectors, zone_sectors))
 		zone_info->nr_zones++;
 
+	if (bdev_is_zoned(bdev) && zone_info->max_zone_append_size == 0) {
+		btrfs_err(fs_info, "zoned: device %pg does not support zone append",
+			  bdev);
+		ret = -EINVAL;
+		goto out;
+	}
+
 	zone_info->seq_zones = bitmap_zalloc(zone_info->nr_zones, GFP_KERNEL);
 	if (!zone_info->seq_zones) {
 		ret = -ENOMEM;