| Message ID | 157845705884.82882.5003824524655587269.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: fix maxbytes problems on 32-bit systems | expand |
> --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -1540,6 +1540,7 @@ typedef struct xfs_bmdr_block { > #define BMBT_BLOCKCOUNT_BITLEN 21 > > #define BMBT_STARTOFF_MASK ((1ULL << BMBT_STARTOFF_BITLEN) - 1) > +#define XFS_MAX_FILEOFF (BMBT_STARTOFF_MASK) I think throwing in a comment here would be useful. Otherwise the patch looks good to me.
On Wed, Jan 08, 2020 at 12:09:27AM -0800, Christoph Hellwig wrote: > > --- a/fs/xfs/libxfs/xfs_format.h > > +++ b/fs/xfs/libxfs/xfs_format.h > > @@ -1540,6 +1540,7 @@ typedef struct xfs_bmdr_block { > > #define BMBT_BLOCKCOUNT_BITLEN 21 > > > > #define BMBT_STARTOFF_MASK ((1ULL << BMBT_STARTOFF_BITLEN) - 1) > > +#define XFS_MAX_FILEOFF (BMBT_STARTOFF_MASK) > > I think throwing in a comment here would be useful. > > Otherwise the patch looks good to me. Ok, done. --D
On Tue, Jan 07, 2020 at 08:17:38PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Introduce a new #define for the maximum supported file block offset. > We'll use this in the next patch to make it more obvious that we're > doing some operation for all possible inode fork mappings after a given > offset. We can't use ULLONG_MAX here because bunmapi uses that to > detect when it's done. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/libxfs/xfs_format.h | 1 + > fs/xfs/xfs_reflink.c | 3 ++- > 2 files changed, 3 insertions(+), 1 deletion(-) > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 1b7dcbae051c..c2976e441d43 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -1540,6 +1540,7 @@ typedef struct xfs_bmdr_block { > #define BMBT_BLOCKCOUNT_BITLEN 21 > > #define BMBT_STARTOFF_MASK ((1ULL << BMBT_STARTOFF_BITLEN) - 1) > +#define XFS_MAX_FILEOFF (BMBT_STARTOFF_MASK) Isn't the maximum file offset in the BMBT the max start offset + the max length of the extent that is located at BMBT_STARTOFF_MASK? Cheers, Dave.
On Thu, Jan 09, 2020 at 07:40:41AM +1100, Dave Chinner wrote: > On Tue, Jan 07, 2020 at 08:17:38PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Introduce a new #define for the maximum supported file block offset. > > We'll use this in the next patch to make it more obvious that we're > > doing some operation for all possible inode fork mappings after a given > > offset. We can't use ULLONG_MAX here because bunmapi uses that to > > detect when it's done. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/xfs/libxfs/xfs_format.h | 1 + > > fs/xfs/xfs_reflink.c | 3 ++- > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > > index 1b7dcbae051c..c2976e441d43 100644 > > --- a/fs/xfs/libxfs/xfs_format.h > > +++ b/fs/xfs/libxfs/xfs_format.h > > @@ -1540,6 +1540,7 @@ typedef struct xfs_bmdr_block { > > #define BMBT_BLOCKCOUNT_BITLEN 21 > > > > #define BMBT_STARTOFF_MASK ((1ULL << BMBT_STARTOFF_BITLEN) - 1) > > +#define XFS_MAX_FILEOFF (BMBT_STARTOFF_MASK) > > Isn't the maximum file offset in the BMBT the max start offset + the > max length of the extent that is located at BMBT_STARTOFF_MASK? Apologies for responding to a question with another question, but has there ever been an XFS that supported an inode size of more than 8EB? Linux supports at most a file offset of 8EB, which is 2^63-1, or 0x7FFF,FFFF,FFFF,FFFF. On a filesystem with 512-byte blocks, the very last byte in the file would be in block 2^54-1, or 0x3F,FFFF,FFFF,FFFF. Larger blocksizes decrease that even further (e.g. 2^47-1, or 0x7FFF,FFFF,FFFF on 64k block filesystems). Therefore, on Linux I conclude that the largest file offset (block) possible is 2^54-1, which is BMBT_STARTOFF_MASK. Unless there's an XFS port that actually supports 16EB files, BMBT_STARTOFF_MASK will suffice here. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Wed, Jan 08, 2020 at 02:32:38PM -0800, Darrick J. Wong wrote: > On Thu, Jan 09, 2020 at 07:40:41AM +1100, Dave Chinner wrote: > > On Tue, Jan 07, 2020 at 08:17:38PM -0800, Darrick J. Wong wrote: > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > Introduce a new #define for the maximum supported file block offset. > > > We'll use this in the next patch to make it more obvious that we're > > > doing some operation for all possible inode fork mappings after a given > > > offset. We can't use ULLONG_MAX here because bunmapi uses that to > > > detect when it's done. > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > fs/xfs/libxfs/xfs_format.h | 1 + > > > fs/xfs/xfs_reflink.c | 3 ++- > > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > > > index 1b7dcbae051c..c2976e441d43 100644 > > > --- a/fs/xfs/libxfs/xfs_format.h > > > +++ b/fs/xfs/libxfs/xfs_format.h > > > @@ -1540,6 +1540,7 @@ typedef struct xfs_bmdr_block { > > > #define BMBT_BLOCKCOUNT_BITLEN 21 > > > > > > #define BMBT_STARTOFF_MASK ((1ULL << BMBT_STARTOFF_BITLEN) - 1) > > > +#define XFS_MAX_FILEOFF (BMBT_STARTOFF_MASK) > > > > Isn't the maximum file offset in the BMBT the max start offset + the > > max length of the extent that is located at BMBT_STARTOFF_MASK? > > Apologies for responding to a question with another question, but has > there ever been an XFS that supported an inode size of more than 8EB? Doubt it. > Linux supports at most a file offset of 8EB, which is 2^63-1, or > 0x7FFF,FFFF,FFFF,FFFF. On a filesystem with 512-byte blocks, the very > last byte in the file would be in block 2^54-1, or 0x3F,FFFF,FFFF,FFFF. > Larger blocksizes decrease that even further (e.g. 2^47-1, or > 0x7FFF,FFFF,FFFF on 64k block filesystems). > > Therefore, on Linux I conclude that the largest file offset (block) > possible is 2^54-1, which is BMBT_STARTOFF_MASK. Unless there's an > XFS port that actually supports 16EB files, BMBT_STARTOFF_MASK will > suffice here. Sure, but my point was that checks against the max file offset as a block count are applied to the startoff field, not the startoff + blockcount value, so we can potentially get extents on disk beyond the above definition of XFS_MAX_FILEOFF... i.e. startoff can be < XFS_MAX_FILEOFF, but startoff + blockcount can be > XFS_MAX_FILEOFF, and there's nothing in the code that prevents that from occurring... e.g. what's preventing speculative delalloc from going beyond XFS_MAX_FILEOFF, even though the actual file offset that is being written is within XFS_MAX_FILEOFF? Cheers, Dave.
On Thu, Jan 09, 2020 at 09:42:16AM +1100, Dave Chinner wrote: > On Wed, Jan 08, 2020 at 02:32:38PM -0800, Darrick J. Wong wrote: > > On Thu, Jan 09, 2020 at 07:40:41AM +1100, Dave Chinner wrote: > > > On Tue, Jan 07, 2020 at 08:17:38PM -0800, Darrick J. Wong wrote: > > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > > > Introduce a new #define for the maximum supported file block offset. > > > > We'll use this in the next patch to make it more obvious that we're > > > > doing some operation for all possible inode fork mappings after a given > > > > offset. We can't use ULLONG_MAX here because bunmapi uses that to > > > > detect when it's done. > > > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > --- > > > > fs/xfs/libxfs/xfs_format.h | 1 + > > > > fs/xfs/xfs_reflink.c | 3 ++- > > > > 2 files changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > > > > index 1b7dcbae051c..c2976e441d43 100644 > > > > --- a/fs/xfs/libxfs/xfs_format.h > > > > +++ b/fs/xfs/libxfs/xfs_format.h > > > > @@ -1540,6 +1540,7 @@ typedef struct xfs_bmdr_block { > > > > #define BMBT_BLOCKCOUNT_BITLEN 21 > > > > > > > > #define BMBT_STARTOFF_MASK ((1ULL << BMBT_STARTOFF_BITLEN) - 1) > > > > +#define XFS_MAX_FILEOFF (BMBT_STARTOFF_MASK) > > > > > > Isn't the maximum file offset in the BMBT the max start offset + the > > > max length of the extent that is located at BMBT_STARTOFF_MASK? > > > > Apologies for responding to a question with another question, but has > > there ever been an XFS that supported an inode size of more than 8EB? > > Doubt it. > > > Linux supports at most a file offset of 8EB, which is 2^63-1, or > > 0x7FFF,FFFF,FFFF,FFFF. On a filesystem with 512-byte blocks, the very > > last byte in the file would be in block 2^54-1, or 0x3F,FFFF,FFFF,FFFF. > > Larger blocksizes decrease that even further (e.g. 2^47-1, or > > 0x7FFF,FFFF,FFFF on 64k block filesystems). > > > > Therefore, on Linux I conclude that the largest file offset (block) > > possible is 2^54-1, which is BMBT_STARTOFF_MASK. Unless there's an > > XFS port that actually supports 16EB files, BMBT_STARTOFF_MASK will > > suffice here. > > Sure, but my point was that checks against the max file offset > as a block count are applied to the startoff field, not the > startoff + blockcount value, so we can potentially get extents on > disk beyond the above definition of XFS_MAX_FILEOFF... > > i.e. startoff can be < XFS_MAX_FILEOFF, but startoff + blockcount > can be > XFS_MAX_FILEOFF, and there's nothing in the code that > prevents that from occurring... > > e.g. what's preventing speculative delalloc from going beyond > XFS_MAX_FILEOFF, even though the actual file offset that is being > written is within XFS_MAX_FILEOFF? I thought we constrained the @prealloc_blocks argument to xfs_bmapi_reserve_delalloc based on s_maxbytes: p_end_fsb = min(p_end_fsb, XFS_B_TO_FSB(mp, mp->m_super->s_maxbytes)); But as there's nothing in the verifiers or anywhere else prohibiting this, I'll change it to (BMBT_STARTOFF_MASK + MAXEXTLEN) for now. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 1b7dcbae051c..c2976e441d43 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -1540,6 +1540,7 @@ typedef struct xfs_bmdr_block { #define BMBT_BLOCKCOUNT_BITLEN 21 #define BMBT_STARTOFF_MASK ((1ULL << BMBT_STARTOFF_BITLEN) - 1) +#define XFS_MAX_FILEOFF (BMBT_STARTOFF_MASK) typedef struct xfs_bmbt_rec { __be64 l0, l1; diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c index de451235c4ee..7a6c94295b8a 100644 --- a/fs/xfs/xfs_reflink.c +++ b/fs/xfs/xfs_reflink.c @@ -1457,7 +1457,8 @@ xfs_reflink_clear_inode_flag( * We didn't find any shared blocks so turn off the reflink flag. * First, get rid of any leftover CoW mappings. */ - error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, NULLFILEOFF, true); + error = xfs_reflink_cancel_cow_blocks(ip, tpp, 0, XFS_MAX_FILEOFF, + true); if (error) return error;