[1/3] xfs: introduce XFS_MAX_FILEOFF
diff mbox series

Message ID 157845705884.82882.5003824524655587269.stgit@magnolia
State Superseded
Headers show
Series
  • xfs: fix maxbytes problems on 32-bit systems
Related show

Commit Message

Darrick J. Wong Jan. 8, 2020, 4:17 a.m. UTC
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(-)

Comments

Christoph Hellwig Jan. 8, 2020, 8:09 a.m. UTC | #1
> --- 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.
Darrick J. Wong Jan. 8, 2020, 4:37 p.m. UTC | #2
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
Dave Chinner Jan. 8, 2020, 8:40 p.m. UTC | #3
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.
Darrick J. Wong Jan. 8, 2020, 10:32 p.m. UTC | #4
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
Dave Chinner Jan. 8, 2020, 10:42 p.m. UTC | #5
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.
Darrick J. Wong Jan. 8, 2020, 11:04 p.m. UTC | #6
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

Patch
diff mbox series

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;