diff mbox series

[1/2] xfs: don't catch dax+reflink inodes as corruption in verifier

Message ID 4b655a26-0e3c-3da7-2017-6ed88a46a611@redhat.com (mailing list archive)
State Accepted, archived
Headers show
Series xfs: fix up some reflink+dax interactions | expand

Commit Message

Eric Sandeen Dec. 1, 2020, 7:16 p.m. UTC
We don't yet support dax on reflinked files, but that is in the works.

Further, having the flag set does not automatically mean that the inode
is actually "in the CPU direct access state," which depends on several
other conditions in addition to the flag being set.

As such, we should not catch this as corruption in the verifier - simply
not actually enabling S_DAX on reflinked files is enough for now.

Fixes: 4f435ebe7d04 ("xfs: don't mix reflink and DAX mode for now")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_inode_buf.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Christoph Hellwig Dec. 2, 2020, 10:16 a.m. UTC | #1
On Tue, Dec 01, 2020 at 01:16:09PM -0600, Eric Sandeen wrote:
> We don't yet support dax on reflinked files, but that is in the works.
> 
> Further, having the flag set does not automatically mean that the inode
> is actually "in the CPU direct access state," which depends on several
> other conditions in addition to the flag being set.
> 
> As such, we should not catch this as corruption in the verifier - simply
> not actually enabling S_DAX on reflinked files is enough for now.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Dec. 3, 2020, 9:44 p.m. UTC | #2
On Tue, Dec 01, 2020 at 01:16:09PM -0600, Eric Sandeen wrote:
> We don't yet support dax on reflinked files, but that is in the works.
> 
> Further, having the flag set does not automatically mean that the inode
> is actually "in the CPU direct access state," which depends on several
> other conditions in addition to the flag being set.
> 
> As such, we should not catch this as corruption in the verifier - simply
> not actually enabling S_DAX on reflinked files is enough for now.
> 
> Fixes: 4f435ebe7d04 ("xfs: don't mix reflink and DAX mode for now")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_inode_buf.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index c667c63f2cb0..4d7410e49db4 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -547,10 +547,6 @@ xfs_dinode_verify(
>  	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags & XFS_DIFLAG_REALTIME))
>  		return __this_address;
>  
> -	/* don't let reflink and dax mix */
> -	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
> -		return __this_address;

If we're going to let in inodes with the DAX and REFLINK iflags set,
doesn't that mean that xfs_inode_should_enable_dax needs to return false
if REFLINK is set?

--D

> -
>  	/* COW extent size hint validation */
>  	fa = xfs_inode_validate_cowextsize(mp, be32_to_cpu(dip->di_cowextsize),
>  			mode, flags, flags2);
> -- 
> 2.17.0
>
Eric Sandeen Dec. 3, 2020, 10:19 p.m. UTC | #3
On 12/3/20 3:44 PM, Darrick J. Wong wrote:
> On Tue, Dec 01, 2020 at 01:16:09PM -0600, Eric Sandeen wrote:
>> We don't yet support dax on reflinked files, but that is in the works.
>>
>> Further, having the flag set does not automatically mean that the inode
>> is actually "in the CPU direct access state," which depends on several
>> other conditions in addition to the flag being set.
>>
>> As such, we should not catch this as corruption in the verifier - simply
>> not actually enabling S_DAX on reflinked files is enough for now.
>>
>> Fixes: 4f435ebe7d04 ("xfs: don't mix reflink and DAX mode for now")
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>  fs/xfs/libxfs/xfs_inode_buf.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
>> index c667c63f2cb0..4d7410e49db4 100644
>> --- a/fs/xfs/libxfs/xfs_inode_buf.c
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
>> @@ -547,10 +547,6 @@ xfs_dinode_verify(
>>  	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags & XFS_DIFLAG_REALTIME))
>>  		return __this_address;
>>  
>> -	/* don't let reflink and dax mix */
>> -	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
>> -		return __this_address;
> 
> If we're going to let in inodes with the DAX and REFLINK iflags set,
> doesn't that mean that xfs_inode_should_enable_dax needs to return false
> if REFLINK is set?

I think it does already, no?

static bool
xfs_inode_should_enable_dax(
        struct xfs_inode *ip)
{
        if (!IS_ENABLED(CONFIG_FS_DAX))
                return false;
        if (ip->i_mount->m_flags & XFS_MOUNT_DAX_NEVER)
                return false;
        if (!xfs_inode_supports_dax(ip)) <------
                return false;


----> xfs_inode_supports_dax ---> 

static bool
xfs_inode_supports_dax(
        struct xfs_inode        *ip)
{
        struct xfs_mount        *mp = ip->i_mount;

        /* Only supported on regular files. */
        if (!S_ISREG(VFS_I(ip)->i_mode))
                return false;

        /* Only supported on non-reflinked files. */
        if (xfs_is_reflink_inode(ip))
                return false;
Darrick J. Wong Dec. 3, 2020, 11:03 p.m. UTC | #4
On Thu, Dec 03, 2020 at 04:19:35PM -0600, Eric Sandeen wrote:
> On 12/3/20 3:44 PM, Darrick J. Wong wrote:
> > On Tue, Dec 01, 2020 at 01:16:09PM -0600, Eric Sandeen wrote:
> >> We don't yet support dax on reflinked files, but that is in the works.
> >>
> >> Further, having the flag set does not automatically mean that the inode
> >> is actually "in the CPU direct access state," which depends on several
> >> other conditions in addition to the flag being set.
> >>
> >> As such, we should not catch this as corruption in the verifier - simply
> >> not actually enabling S_DAX on reflinked files is enough for now.
> >>
> >> Fixes: 4f435ebe7d04 ("xfs: don't mix reflink and DAX mode for now")
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>  fs/xfs/libxfs/xfs_inode_buf.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> >> index c667c63f2cb0..4d7410e49db4 100644
> >> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> >> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> >> @@ -547,10 +547,6 @@ xfs_dinode_verify(
> >>  	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags & XFS_DIFLAG_REALTIME))
> >>  		return __this_address;
> >>  
> >> -	/* don't let reflink and dax mix */
> >> -	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
> >> -		return __this_address;
> > 
> > If we're going to let in inodes with the DAX and REFLINK iflags set,
> > doesn't that mean that xfs_inode_should_enable_dax needs to return false
> > if REFLINK is set?
> 
> I think it does already, no?
> 
> static bool
> xfs_inode_should_enable_dax(
>         struct xfs_inode *ip)
> {
>         if (!IS_ENABLED(CONFIG_FS_DAX))
>                 return false;
>         if (ip->i_mount->m_flags & XFS_MOUNT_DAX_NEVER)
>                 return false;
>         if (!xfs_inode_supports_dax(ip)) <------
>                 return false;
> 
> 
> ----> xfs_inode_supports_dax ---> 
> 
> static bool
> xfs_inode_supports_dax(
>         struct xfs_inode        *ip)
> {
>         struct xfs_mount        *mp = ip->i_mount;
> 
>         /* Only supported on regular files. */
>         if (!S_ISREG(VFS_I(ip)->i_mode))
>                 return false;
> 
>         /* Only supported on non-reflinked files. */
>         if (xfs_is_reflink_inode(ip))
>                 return false;
> 

DOH  yes it does,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index c667c63f2cb0..4d7410e49db4 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -547,10 +547,6 @@  xfs_dinode_verify(
 	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags & XFS_DIFLAG_REALTIME))
 		return __this_address;
 
-	/* don't let reflink and dax mix */
-	if ((flags2 & XFS_DIFLAG2_REFLINK) && (flags2 & XFS_DIFLAG2_DAX))
-		return __this_address;
-
 	/* COW extent size hint validation */
 	fa = xfs_inode_validate_cowextsize(mp, be32_to_cpu(dip->di_cowextsize),
 			mode, flags, flags2);