| Message ID | 1554885748-7071-1-git-send-email-wshilong1991@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs,fstrim: fix to return correct minlen | expand |
On Wed, Apr 10, 2019 at 05:42:28PM +0900, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > This patch tries to address two problems: > > 1) return @minlen we used to trim to > user space. > > 2) return EINVAL if granularity is larger than > avg size, even most of cases, granularity is small(4K), > but if devices return a lager granularity for some reaons > (testing, bugs etc), fstrim should return failure directly. It really is a pity^W^Wannoying that there's no manual page for FITRIM, because I have no idea if this is the right or desired behavior for this ioctl. Do the other filesystems update @minlen so that userspace knows the granularity? --D > Signed-off-by: Wang Shilong <wshilong@ddn.com> > --- > fs/xfs/xfs_discard.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c > index 93f07edafd81..66702b03587a 100644 > --- a/fs/xfs/xfs_discard.c > +++ b/fs/xfs/xfs_discard.c > @@ -164,6 +164,8 @@ xfs_ioc_trim( > if (copy_from_user(&range, urange, sizeof(range))) > return -EFAULT; > > + range.minlen = max_t(u64, granularity, range.minlen); > + minlen = BTOBB(range.minlen); > /* > * Truncating down the len isn't actually quite correct, but using > * BBTOB would mean we trivially get overflows for values > @@ -178,7 +180,6 @@ xfs_ioc_trim( > > start = BTOBB(range.start); > end = start + BTOBBT(range.len) - 1; > - minlen = BTOBB(max_t(u64, granularity, range.minlen)); > > if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1) > end = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks)- 1; > -- > 2.20.1 >
Hi Darrick, On Wed, Apr 10, 2019 at 05:42:28PM +0900, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > This patch tries to address two problems: > > 1) return @minlen we used to trim to > user space. > > 2) return EINVAL if granularity is larger than > avg size, even most of cases, granularity is small(4K), > but if devices return a lager granularity for some reaons > (testing, bugs etc), fstrim should return failure directly. It really is a pity^W^Wannoying that there's no manual page for FITRIM, because I have no idea if this is the right or desired behavior for this ioctl. Do the other filesystems update @minlen so that userspace knows the granularity? --D ---------> As far as I checked ext4, f2fs, Btrfs did so, I think it makes sense we return this value to userspace, we'd better to doc it somewhere 'man fstrim'? thanks, Shilong
On Thu, Apr 11, 2019 at 02:05:08AM +0000, Wang Shilong wrote: > Hi Darrick, > > > On Wed, Apr 10, 2019 at 05:42:28PM +0900, Wang Shilong wrote: > > From: Wang Shilong <wshilong@ddn.com> > > > > This patch tries to address two problems: > > > > 1) return @minlen we used to trim to > > user space. > > > > 2) return EINVAL if granularity is larger than > > avg size, even most of cases, granularity is small(4K), > > but if devices return a lager granularity for some reaons > > (testing, bugs etc), fstrim should return failure directly. > > It really is a pity^W^Wannoying that there's no manual page for FITRIM, > because I have no idea if this is the right or desired behavior for this > ioctl. > > Do the other filesystems update @minlen so that userspace knows the > granularity? > > --D > > ---------> > As far as I checked ext4, f2fs, Btrfs did so, Fair enough. > I think it makes sense we return this value to userspace, we'd better > to doc it somewhere 'man fstrim'? Ok, please cc me when you send the documentation update. --D > thanks, > Shilong
On Wed, Apr 10, 2019 at 05:42:28PM +0900, Wang Shilong wrote: > From: Wang Shilong <wshilong@ddn.com> > > This patch tries to address two problems: > > 1) return @minlen we used to trim to > user space. > > 2) return EINVAL if granularity is larger than > avg size, even most of cases, granularity is small(4K), > but if devices return a lager granularity for some reaons > (testing, bugs etc), fstrim should return failure directly. > > Signed-off-by: Wang Shilong <wshilong@ddn.com> Ok, well, I guess all the other filesystems do this, so we should too... Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --- > fs/xfs/xfs_discard.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c > index 93f07edafd81..66702b03587a 100644 > --- a/fs/xfs/xfs_discard.c > +++ b/fs/xfs/xfs_discard.c > @@ -164,6 +164,8 @@ xfs_ioc_trim( > if (copy_from_user(&range, urange, sizeof(range))) > return -EFAULT; > > + range.minlen = max_t(u64, granularity, range.minlen); > + minlen = BTOBB(range.minlen); > /* > * Truncating down the len isn't actually quite correct, but using > * BBTOB would mean we trivially get overflows for values > @@ -178,7 +180,6 @@ xfs_ioc_trim( > > start = BTOBB(range.start); > end = start + BTOBBT(range.len) - 1; > - minlen = BTOBB(max_t(u64, granularity, range.minlen)); > > if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1) > end = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks)- 1; > -- > 2.20.1 >
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c index 93f07edafd81..66702b03587a 100644 --- a/fs/xfs/xfs_discard.c +++ b/fs/xfs/xfs_discard.c @@ -164,6 +164,8 @@ xfs_ioc_trim( if (copy_from_user(&range, urange, sizeof(range))) return -EFAULT; + range.minlen = max_t(u64, granularity, range.minlen); + minlen = BTOBB(range.minlen); /* * Truncating down the len isn't actually quite correct, but using * BBTOB would mean we trivially get overflows for values @@ -178,7 +180,6 @@ xfs_ioc_trim( start = BTOBB(range.start); end = start + BTOBBT(range.len) - 1; - minlen = BTOBB(max_t(u64, granularity, range.minlen)); if (end > XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) - 1) end = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks)- 1;