| Message ID | 20240115193859.1521-1-dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: don't warn if discard range is not aligned to sector | expand |
On 15.01.24 20:39, David Sterba wrote: > There's a warning in btrfs_issue_discard() when the range is not aligned > to 512 bytes, originally added in 4d89d377bbb0 ("btrfs: > btrfs_issue_discard ensure offset/length are aligned to sector > boundaries"). We can't do sub-sector writes anyway so the adjustment is > the only thing that we can do and the warning is unnecessary. > > CC: stable@vger.kernel.org # 4.19+ > Reported-by: syzbot+4a4f1eba14eb5c3417d1@syzkaller.appspotmail.com > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/extent-tree.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 6d680031211a..8e8cc1111277 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -1260,7 +1260,8 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len, > u64 bytes_left, end; > u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT); > > - if (WARN_ON(start != aligned_start)) { > + /* Adjust the range to be aligned to 512B sectors if necessary. */ > + if (start != aligned_start) { > len -= aligned_start - start; > len = round_down(len, 1 << SECTOR_SHIFT); > start = aligned_start; Maybe leave a btrfs_warn(fs_info, "adjusting discard range to 512b boundary"); in there? Or is info a more appropriate level?
On 2024/1/16 06:08, David Sterba wrote: > There's a warning in btrfs_issue_discard() when the range is not aligned > to 512 bytes, originally added in 4d89d377bbb0 ("btrfs: > btrfs_issue_discard ensure offset/length are aligned to sector > boundaries"). We can't do sub-sector writes anyway so the adjustment is > the only thing that we can do and the warning is unnecessary. > > CC: stable@vger.kernel.org # 4.19+ > Reported-by: syzbot+4a4f1eba14eb5c3417d1@syzkaller.appspotmail.com > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/extent-tree.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 6d680031211a..8e8cc1111277 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -1260,7 +1260,8 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len, > u64 bytes_left, end; > u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT); > > - if (WARN_ON(start != aligned_start)) { > + /* Adjust the range to be aligned to 512B sectors if necessary. */ > + if (start != aligned_start) { > len -= aligned_start - start; > len = round_down(len, 1 << SECTOR_SHIFT); > start = aligned_start; Can we do one step further in mkfs and device add, by rounding down the device size to btrfs sector boundary? Add maybe output a warning message at mount time when adding one device? Thanks, Qu
On Tue, Jan 16, 2024 at 07:42:39PM +1030, Qu Wenruo wrote: > > > On 2024/1/16 06:08, David Sterba wrote: > > There's a warning in btrfs_issue_discard() when the range is not aligned > > to 512 bytes, originally added in 4d89d377bbb0 ("btrfs: > > btrfs_issue_discard ensure offset/length are aligned to sector > > boundaries"). We can't do sub-sector writes anyway so the adjustment is > > the only thing that we can do and the warning is unnecessary. > > > > CC: stable@vger.kernel.org # 4.19+ > > Reported-by: syzbot+4a4f1eba14eb5c3417d1@syzkaller.appspotmail.com > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/extent-tree.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > > index 6d680031211a..8e8cc1111277 100644 > > --- a/fs/btrfs/extent-tree.c > > +++ b/fs/btrfs/extent-tree.c > > @@ -1260,7 +1260,8 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len, > > u64 bytes_left, end; > > u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT); > > > > - if (WARN_ON(start != aligned_start)) { > > + /* Adjust the range to be aligned to 512B sectors if necessary. */ > > + if (start != aligned_start) { > > len -= aligned_start - start; > > len = round_down(len, 1 << SECTOR_SHIFT); > > start = aligned_start; > Can we do one step further in mkfs and device add, by rounding down the > device size to btrfs sector boundary? This is not device size but the range that gets passed to discard. The start and size get converted to the 512B units anyway when calling blkdev_issue_discard(), so nothing else needs to be done.
On Tue, Jan 16, 2024 at 08:40:23AM +0000, Johannes Thumshirn wrote: > On 15.01.24 20:39, David Sterba wrote: > > There's a warning in btrfs_issue_discard() when the range is not aligned > > to 512 bytes, originally added in 4d89d377bbb0 ("btrfs: > > btrfs_issue_discard ensure offset/length are aligned to sector > > boundaries"). We can't do sub-sector writes anyway so the adjustment is > > the only thing that we can do and the warning is unnecessary. > > > > CC: stable@vger.kernel.org # 4.19+ > > Reported-by: syzbot+4a4f1eba14eb5c3417d1@syzkaller.appspotmail.com > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/extent-tree.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > > index 6d680031211a..8e8cc1111277 100644 > > --- a/fs/btrfs/extent-tree.c > > +++ b/fs/btrfs/extent-tree.c > > @@ -1260,7 +1260,8 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len, > > u64 bytes_left, end; > > u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT); > > > > - if (WARN_ON(start != aligned_start)) { > > + /* Adjust the range to be aligned to 512B sectors if necessary. */ > > + if (start != aligned_start) { > > len -= aligned_start - start; > > len = round_down(len, 1 << SECTOR_SHIFT); > > start = aligned_start; > > Maybe leave a > btrfs_warn(fs_info, "adjusting discard range to 512b boundary"); > in there? > > Or is info a more appropriate level? I don't know if we should warn here at all, the warning is there since 2015 and only now we see it triggered, I guess everybody sane using the discard interface uses proper values. Warning level means that it needs user attention that something is wrong and take some action or examine the system. The discard range could be verified at the beginning of the ioctl, but printing it here is a bit late. I've checked xfs and it silently rounds down the start and offset the same way, I find this sufficient thing to do.
On 16.01.24 16:54, David Sterba wrote: > > I don't know if we should warn here at all, the warning is there since > 2015 and only now we see it triggered, I guess everybody sane using the > discard interface uses proper values. Warning level means that it needs > user attention that something is wrong and take some action or examine > the system. > > The discard range could be verified at the beginning of the ioctl, but > printing it here is a bit late. I've checked xfs and it silently rounds > down the start and offset the same way, I find this sufficient thing to > do. > Yeah thenm lets do it this way, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> - if (WARN_ON(start != aligned_start)) { > + /* Adjust the range to be aligned to 512B sectors if necessary. */ > + if (start != aligned_start) { Reviewed-by: Anand Jain <anand.jain@oracle.com> Thanks, Anand
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 6d680031211a..8e8cc1111277 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -1260,7 +1260,8 @@ static int btrfs_issue_discard(struct block_device *bdev, u64 start, u64 len, u64 bytes_left, end; u64 aligned_start = ALIGN(start, 1 << SECTOR_SHIFT); - if (WARN_ON(start != aligned_start)) { + /* Adjust the range to be aligned to 512B sectors if necessary. */ + if (start != aligned_start) { len -= aligned_start - start; len = round_down(len, 1 << SECTOR_SHIFT); start = aligned_start;
There's a warning in btrfs_issue_discard() when the range is not aligned to 512 bytes, originally added in 4d89d377bbb0 ("btrfs: btrfs_issue_discard ensure offset/length are aligned to sector boundaries"). We can't do sub-sector writes anyway so the adjustment is the only thing that we can do and the warning is unnecessary. CC: stable@vger.kernel.org # 4.19+ Reported-by: syzbot+4a4f1eba14eb5c3417d1@syzkaller.appspotmail.com Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/extent-tree.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)