| Message ID | 20240313170253.2324812-11-kernel@pankajraghav.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | enable bs > ps in XFS | expand |
On Wed, Mar 13, 2024 at 06:02:52PM +0100, Pankaj Raghav (Samsung) wrote: > From: Pankaj Raghav <p.raghav@samsung.com> > > Instead of assuming that PAGE_SHIFT is always higher than the blocklog, > make the calculation generic so that page cache count can be calculated > correctly for LBS. > > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > --- > fs/xfs/xfs_mount.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index aabb25dc3efa..9cf800586da7 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -133,9 +133,16 @@ xfs_sb_validate_fsb_count( > { > ASSERT(PAGE_SHIFT >= sbp->sb_blocklog); but ... you're still asserting that PAGE_SHIFT is larger than blocklog. Shouldn't you delete that assertion? > ASSERT(sbp->sb_blocklog >= BBSHIFT); > + uint64_t max_index; > + uint64_t max_bytes; > + > + if (check_shl_overflow(nblocks, sbp->sb_blocklog, &max_bytes)) > + return -EFBIG; > > /* Limited by ULONG_MAX of page cache index */ > - if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX) > + max_index = max_bytes >> PAGE_SHIFT; > + > + if (max_index > ULONG_MAX) > return -EFBIG; This kind of depends on the implementation details of the page cache. We have MAX_LFS_FILESIZE to abstract that; maybe that should be used here?
On Mon, Mar 25, 2024 at 07:15:59PM +0000, Matthew Wilcox wrote: > On Wed, Mar 13, 2024 at 06:02:52PM +0100, Pankaj Raghav (Samsung) wrote: > > From: Pankaj Raghav <p.raghav@samsung.com> > > > > Instead of assuming that PAGE_SHIFT is always higher than the blocklog, > > make the calculation generic so that page cache count can be calculated > > correctly for LBS. > > > > Signed-off-by: Pankaj Raghav <p.raghav@samsung.com> > > --- > > fs/xfs/xfs_mount.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > > index aabb25dc3efa..9cf800586da7 100644 > > --- a/fs/xfs/xfs_mount.c > > +++ b/fs/xfs/xfs_mount.c > > @@ -133,9 +133,16 @@ xfs_sb_validate_fsb_count( > > { > > ASSERT(PAGE_SHIFT >= sbp->sb_blocklog); > > but ... you're still asserting that PAGE_SHIFT is larger than blocklog. > Shouldn't you delete that assertion? I do that in the next patch when we enable LBS support in XFS by setting min order. So we still need the check here that will be removed in the next patch. > > > ASSERT(sbp->sb_blocklog >= BBSHIFT); > > + uint64_t max_index; > > + uint64_t max_bytes; > > + > > + if (check_shl_overflow(nblocks, sbp->sb_blocklog, &max_bytes)) > > + return -EFBIG; > > > > /* Limited by ULONG_MAX of page cache index */ > > - if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX) > > + max_index = max_bytes >> PAGE_SHIFT; > > + > > + if (max_index > ULONG_MAX) > > return -EFBIG; > > This kind of depends on the implementation details of the page cache. > We have MAX_LFS_FILESIZE to abstract that; maybe that should be used > here? Makes sense. I will use it instead of using ULONG_MAX.
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index aabb25dc3efa..9cf800586da7 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -133,9 +133,16 @@ xfs_sb_validate_fsb_count( { ASSERT(PAGE_SHIFT >= sbp->sb_blocklog); ASSERT(sbp->sb_blocklog >= BBSHIFT); + uint64_t max_index; + uint64_t max_bytes; + + if (check_shl_overflow(nblocks, sbp->sb_blocklog, &max_bytes)) + return -EFBIG; /* Limited by ULONG_MAX of page cache index */ - if (nblocks >> (PAGE_SHIFT - sbp->sb_blocklog) > ULONG_MAX) + max_index = max_bytes >> PAGE_SHIFT; + + if (max_index > ULONG_MAX) return -EFBIG; return 0; }