linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
@ 2021-08-26 17:30 Bill O'Donnell
  2021-08-26 18:09 ` Darrick J. Wong
  0 siblings, 1 reply; 12+ messages in thread
From: Bill O'Donnell @ 2021-08-26 17:30 UTC (permalink / raw)
  To: linux-xfs

When dax-mode was tri-stated by adding dax=inode case, the EXPERIMENTAL
warning on mount was missed for the case. Add logic to handle the
warning similar to that of the 'dax=always' case.

Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
 fs/xfs/xfs_mount.h | 2 ++
 fs/xfs/xfs_super.c | 8 +++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index e091f3b3fa15..c9243a1b8d05 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -277,6 +277,7 @@ typedef struct xfs_mount {
 #define XFS_FEAT_NEEDSREPAIR	(1ULL << 25)	/* needs xfs_repair */
 
 /* Mount features */
+#define XFS_FEAT_DAX_INODE	(1ULL << 47)	/* DAX enabled */
 #define XFS_FEAT_NOATTR2	(1ULL << 48)	/* disable attr2 creation */
 #define XFS_FEAT_NOALIGN	(1ULL << 49)	/* ignore alignment */
 #define XFS_FEAT_ALLOCSIZE	(1ULL << 50)	/* user specified allocation size */
@@ -359,6 +360,7 @@ __XFS_HAS_FEAT(swalloc, SWALLOC)
 __XFS_HAS_FEAT(filestreams, FILESTREAMS)
 __XFS_HAS_FEAT(dax_always, DAX_ALWAYS)
 __XFS_HAS_FEAT(dax_never, DAX_NEVER)
+__XFS_HAS_FEAT(dax_inode, DAX_INODE)
 __XFS_HAS_FEAT(norecovery, NORECOVERY)
 __XFS_HAS_FEAT(nouuid, NOUUID)
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 5e73ac78bf2f..f73f3687f0a8 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -84,15 +84,16 @@ xfs_mount_set_dax_mode(
 {
 	switch (mode) {
 	case XFS_DAX_INODE:
+		mp->m_features |= XFS_FEAT_DAX_INODE;
 		mp->m_features &= ~(XFS_FEAT_DAX_ALWAYS | XFS_FEAT_DAX_NEVER);
 		break;
 	case XFS_DAX_ALWAYS:
 		mp->m_features |= XFS_FEAT_DAX_ALWAYS;
-		mp->m_features &= ~XFS_FEAT_DAX_NEVER;
+		mp->m_features &= ~(XFS_FEAT_DAX_NEVER | XFS_FEAT_DAX_INODE);
 		break;
 	case XFS_DAX_NEVER:
 		mp->m_features |= XFS_FEAT_DAX_NEVER;
-		mp->m_features &= ~XFS_FEAT_DAX_ALWAYS;
+		mp->m_features &= ~(XFS_FEAT_DAX_ALWAYS | XFS_FEAT_DAX_INODE);
 		break;
 	}
 }
@@ -189,6 +190,7 @@ xfs_fs_show_options(
 		{ XFS_FEAT_LARGE_IOSIZE,	",largeio" },
 		{ XFS_FEAT_DAX_ALWAYS,		",dax=always" },
 		{ XFS_FEAT_DAX_NEVER,		",dax=never" },
+		{ XFS_FEAT_DAX_INODE,		",dax=inode" },
 		{ 0, NULL }
 	};
 	struct xfs_mount	*mp = XFS_M(root->d_sb);
@@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
 	if (xfs_has_crc(mp))
 		sb->s_flags |= SB_I_VERSION;
 
-	if (xfs_has_dax_always(mp)) {
+	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
 		bool rtdev_is_dax = false, datadev_is_dax;
 
 		xfs_warn(mp,
-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-26 17:30 [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case Bill O'Donnell
@ 2021-08-26 18:09 ` Darrick J. Wong
  2021-08-26 18:14   ` Bill O'Donnell
                     ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Darrick J. Wong @ 2021-08-26 18:09 UTC (permalink / raw)
  To: Bill O'Donnell; +Cc: linux-xfs

On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
> When dax-mode was tri-stated by adding dax=inode case, the EXPERIMENTAL
> warning on mount was missed for the case. Add logic to handle the
> warning similar to that of the 'dax=always' case.
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> ---
>  fs/xfs/xfs_mount.h | 2 ++
>  fs/xfs/xfs_super.c | 8 +++++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index e091f3b3fa15..c9243a1b8d05 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
> @@ -277,6 +277,7 @@ typedef struct xfs_mount {
>  #define XFS_FEAT_NEEDSREPAIR	(1ULL << 25)	/* needs xfs_repair */
>  
>  /* Mount features */
> +#define XFS_FEAT_DAX_INODE	(1ULL << 47)	/* DAX enabled */
>  #define XFS_FEAT_NOATTR2	(1ULL << 48)	/* disable attr2 creation */
>  #define XFS_FEAT_NOALIGN	(1ULL << 49)	/* ignore alignment */
>  #define XFS_FEAT_ALLOCSIZE	(1ULL << 50)	/* user specified allocation size */
> @@ -359,6 +360,7 @@ __XFS_HAS_FEAT(swalloc, SWALLOC)
>  __XFS_HAS_FEAT(filestreams, FILESTREAMS)
>  __XFS_HAS_FEAT(dax_always, DAX_ALWAYS)
>  __XFS_HAS_FEAT(dax_never, DAX_NEVER)
> +__XFS_HAS_FEAT(dax_inode, DAX_INODE)
>  __XFS_HAS_FEAT(norecovery, NORECOVERY)
>  __XFS_HAS_FEAT(nouuid, NOUUID)
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 5e73ac78bf2f..f73f3687f0a8 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -84,15 +84,16 @@ xfs_mount_set_dax_mode(
>  {
>  	switch (mode) {
>  	case XFS_DAX_INODE:
> +		mp->m_features |= XFS_FEAT_DAX_INODE;
>  		mp->m_features &= ~(XFS_FEAT_DAX_ALWAYS | XFS_FEAT_DAX_NEVER);
>  		break;
>  	case XFS_DAX_ALWAYS:
>  		mp->m_features |= XFS_FEAT_DAX_ALWAYS;
> -		mp->m_features &= ~XFS_FEAT_DAX_NEVER;
> +		mp->m_features &= ~(XFS_FEAT_DAX_NEVER | XFS_FEAT_DAX_INODE);
>  		break;
>  	case XFS_DAX_NEVER:
>  		mp->m_features |= XFS_FEAT_DAX_NEVER;
> -		mp->m_features &= ~XFS_FEAT_DAX_ALWAYS;
> +		mp->m_features &= ~(XFS_FEAT_DAX_ALWAYS | XFS_FEAT_DAX_INODE);
>  		break;
>  	}
>  }
> @@ -189,6 +190,7 @@ xfs_fs_show_options(
>  		{ XFS_FEAT_LARGE_IOSIZE,	",largeio" },
>  		{ XFS_FEAT_DAX_ALWAYS,		",dax=always" },
>  		{ XFS_FEAT_DAX_NEVER,		",dax=never" },
> +		{ XFS_FEAT_DAX_INODE,		",dax=inode" },
>  		{ 0, NULL }
>  	};
>  	struct xfs_mount	*mp = XFS_M(root->d_sb);
> @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
>  	if (xfs_has_crc(mp))
>  		sb->s_flags |= SB_I_VERSION;
>  
> -	if (xfs_has_dax_always(mp)) {
> +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {

Er... can't this be done without burning another feature bit by:

	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
				       !xfs_has_dax_never(mp))) {
		...
		xfs_warn(mp, "DAX IS EXPERIMENTAL");
	}

--D

>  		bool rtdev_is_dax = false, datadev_is_dax;
>  
>  		xfs_warn(mp,
> -- 
> 2.31.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-26 18:09 ` Darrick J. Wong
@ 2021-08-26 18:14   ` Bill O'Donnell
  2021-08-26 18:16   ` Eric Sandeen
  2021-08-30 15:55   ` Bill O'Donnell
  2 siblings, 0 replies; 12+ messages in thread
From: Bill O'Donnell @ 2021-08-26 18:14 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Bill O'Donnell, linux-xfs

On Thu, Aug 26, 2021 at 11:09:47AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
> > When dax-mode was tri-stated by adding dax=inode case, the EXPERIMENTAL
> > warning on mount was missed for the case. Add logic to handle the
> > warning similar to that of the 'dax=always' case.
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > ---
> >  fs/xfs/xfs_mount.h | 2 ++
> >  fs/xfs/xfs_super.c | 8 +++++---
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> > index e091f3b3fa15..c9243a1b8d05 100644
> > --- a/fs/xfs/xfs_mount.h
> > +++ b/fs/xfs/xfs_mount.h
> > @@ -277,6 +277,7 @@ typedef struct xfs_mount {
> >  #define XFS_FEAT_NEEDSREPAIR	(1ULL << 25)	/* needs xfs_repair */
> >  
> >  /* Mount features */
> > +#define XFS_FEAT_DAX_INODE	(1ULL << 47)	/* DAX enabled */
> >  #define XFS_FEAT_NOATTR2	(1ULL << 48)	/* disable attr2 creation */
> >  #define XFS_FEAT_NOALIGN	(1ULL << 49)	/* ignore alignment */
> >  #define XFS_FEAT_ALLOCSIZE	(1ULL << 50)	/* user specified allocation size */
> > @@ -359,6 +360,7 @@ __XFS_HAS_FEAT(swalloc, SWALLOC)
> >  __XFS_HAS_FEAT(filestreams, FILESTREAMS)
> >  __XFS_HAS_FEAT(dax_always, DAX_ALWAYS)
> >  __XFS_HAS_FEAT(dax_never, DAX_NEVER)
> > +__XFS_HAS_FEAT(dax_inode, DAX_INODE)
> >  __XFS_HAS_FEAT(norecovery, NORECOVERY)
> >  __XFS_HAS_FEAT(nouuid, NOUUID)
> >  
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 5e73ac78bf2f..f73f3687f0a8 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -84,15 +84,16 @@ xfs_mount_set_dax_mode(
> >  {
> >  	switch (mode) {
> >  	case XFS_DAX_INODE:
> > +		mp->m_features |= XFS_FEAT_DAX_INODE;
> >  		mp->m_features &= ~(XFS_FEAT_DAX_ALWAYS | XFS_FEAT_DAX_NEVER);
> >  		break;
> >  	case XFS_DAX_ALWAYS:
> >  		mp->m_features |= XFS_FEAT_DAX_ALWAYS;
> > -		mp->m_features &= ~XFS_FEAT_DAX_NEVER;
> > +		mp->m_features &= ~(XFS_FEAT_DAX_NEVER | XFS_FEAT_DAX_INODE);
> >  		break;
> >  	case XFS_DAX_NEVER:
> >  		mp->m_features |= XFS_FEAT_DAX_NEVER;
> > -		mp->m_features &= ~XFS_FEAT_DAX_ALWAYS;
> > +		mp->m_features &= ~(XFS_FEAT_DAX_ALWAYS | XFS_FEAT_DAX_INODE);
> >  		break;
> >  	}
> >  }
> > @@ -189,6 +190,7 @@ xfs_fs_show_options(
> >  		{ XFS_FEAT_LARGE_IOSIZE,	",largeio" },
> >  		{ XFS_FEAT_DAX_ALWAYS,		",dax=always" },
> >  		{ XFS_FEAT_DAX_NEVER,		",dax=never" },
> > +		{ XFS_FEAT_DAX_INODE,		",dax=inode" },
> >  		{ 0, NULL }
> >  	};
> >  	struct xfs_mount	*mp = XFS_M(root->d_sb);
> > @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
> >  	if (xfs_has_crc(mp))
> >  		sb->s_flags |= SB_I_VERSION;
> >  
> > -	if (xfs_has_dax_always(mp)) {
> > +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
> 
> Er... can't this be done without burning another feature bit by:
> 
> 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
> 				       !xfs_has_dax_never(mp))) {
> 		...
> 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
> 	}
> 

Good idea. I'll send a v2.
Thanks-
Bill


> --D
> 
> >  		bool rtdev_is_dax = false, datadev_is_dax;
> >  
> >  		xfs_warn(mp,
> > -- 
> > 2.31.1
> > 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-26 18:09 ` Darrick J. Wong
  2021-08-26 18:14   ` Bill O'Donnell
@ 2021-08-26 18:16   ` Eric Sandeen
  2021-08-26 22:08     ` Bill O'Donnell
  2021-08-30 15:55   ` Bill O'Donnell
  2 siblings, 1 reply; 12+ messages in thread
From: Eric Sandeen @ 2021-08-26 18:16 UTC (permalink / raw)
  To: Darrick J. Wong, Bill O'Donnell; +Cc: linux-xfs


On 8/26/21 1:09 PM, Darrick J. Wong wrote:
> On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:

>> @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
>>   	if (xfs_has_crc(mp))
>>   		sb->s_flags |= SB_I_VERSION;
>>   
>> -	if (xfs_has_dax_always(mp)) {
>> +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
> 
> Er... can't this be done without burning another feature bit by:
> 
> 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
> 				       !xfs_has_dax_never(mp))) {
> 		...
> 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
> 	}

changing this conditional in this way will also fail dax=inode mounts on
reflink-capable (i.e. default) filesystems, no?

-	if (xfs_has_dax_always(mp)) {
+	if (xfs_has_dax_always(mp) || $NEW_DAX_INODE_TEST) {

...
                 if (xfs_has_reflink(mp)) {
                         xfs_alert(mp,
                 "DAX and reflink cannot be used together!");
                         error = -EINVAL;
                         goto out_filestream_unmount;
                 }
         }

-Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-26 18:16   ` Eric Sandeen
@ 2021-08-26 22:08     ` Bill O'Donnell
  2021-08-26 23:43       ` Eric Sandeen
  0 siblings, 1 reply; 12+ messages in thread
From: Bill O'Donnell @ 2021-08-26 22:08 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Darrick J. Wong, Bill O'Donnell, linux-xfs

On Thu, Aug 26, 2021 at 01:16:22PM -0500, Eric Sandeen wrote:
> 
> On 8/26/21 1:09 PM, Darrick J. Wong wrote:
> > On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
> 
> > > @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
> > >   	if (xfs_has_crc(mp))
> > >   		sb->s_flags |= SB_I_VERSION;
> > > -	if (xfs_has_dax_always(mp)) {
> > > +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
> > 
> > Er... can't this be done without burning another feature bit by:
> > 
> > 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
> > 				       !xfs_has_dax_never(mp))) {
> > 		...
> > 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
> > 	}
> 
> changing this conditional in this way will also fail dax=inode mounts on
> reflink-capable (i.e. default) filesystems, no?

Correct. My original patch tests fine, and still handles the reflink and dax
incompatibility. The new suggested logic is problematic. 
-Bill

> 
> -	if (xfs_has_dax_always(mp)) {
> +	if (xfs_has_dax_always(mp) || $NEW_DAX_INODE_TEST) {
> 
> ...
>                 if (xfs_has_reflink(mp)) {
>                         xfs_alert(mp,
>                 "DAX and reflink cannot be used together!");
>                         error = -EINVAL;
>                         goto out_filestream_unmount;
>                 }
>         }
> 
> -Eric
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-26 22:08     ` Bill O'Donnell
@ 2021-08-26 23:43       ` Eric Sandeen
  2021-08-27 14:03         ` Bill O'Donnell
  0 siblings, 1 reply; 12+ messages in thread
From: Eric Sandeen @ 2021-08-26 23:43 UTC (permalink / raw)
  To: Bill O'Donnell; +Cc: Darrick J. Wong, Bill O'Donnell, linux-xfs

On 8/26/21 5:08 PM, Bill O'Donnell wrote:
> On Thu, Aug 26, 2021 at 01:16:22PM -0500, Eric Sandeen wrote:
>>
>> On 8/26/21 1:09 PM, Darrick J. Wong wrote:
>>> On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
>>
>>>> @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
>>>>    	if (xfs_has_crc(mp))
>>>>    		sb->s_flags |= SB_I_VERSION;
>>>> -	if (xfs_has_dax_always(mp)) {
>>>> +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
>>>
>>> Er... can't this be done without burning another feature bit by:
>>>
>>> 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
>>> 				       !xfs_has_dax_never(mp))) {
>>> 		...
>>> 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
>>> 	}
>>
>> changing this conditional in this way will also fail dax=inode mounts on
>> reflink-capable (i.e. default) filesystems, no?
> 
> Correct. My original patch tests fine, and still handles the reflink and dax
> incompatibility. The new suggested logic is problematic.
> -Bill

I think that both your proposed patch and Darrick's suggestion have this problem.

"mount -o dax=inode" makes your new xfs_has_dax_inode(mp) true, and in that
conditional, if the filesystem has reflink enabled, mount fails:

# mkfs.xfs -f /dev/pmem0p1
meta-data=/dev/pmem0p1           isize=512    agcount=4, agsize=4194304 blks
          =                       sectsz=4096  attr=2, projid32bit=1
          =                       crc=1        finobt=1, sparse=1, rmapbt=0
          =                       reflink=1    bigtime=0 inobtcount=0
data     =                       bsize=4096   blocks=16777216, imaxpct=25
          =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
log      =internal log           bsize=4096   blocks=8192, version=2
          =                       sectsz=4096  sunit=1 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0

# mount -o dax=inode /dev/pmem0p1 /mnt/test
mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1,
        missing codepage or helper program, or other error

        In some cases useful info is found in syslog - try
        dmesg | tail or so.

# dmesg | tail -n 2
[  192.691733] XFS (pmem0p1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
[  192.700300] XFS (pmem0p1): DAX and reflink cannot be used together!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-26 23:43       ` Eric Sandeen
@ 2021-08-27 14:03         ` Bill O'Donnell
  2021-08-27 14:18           ` Eric Sandeen
  0 siblings, 1 reply; 12+ messages in thread
From: Bill O'Donnell @ 2021-08-27 14:03 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Darrick J. Wong, Bill O'Donnell, linux-xfs

On Thu, Aug 26, 2021 at 06:43:44PM -0500, Eric Sandeen wrote:
> On 8/26/21 5:08 PM, Bill O'Donnell wrote:
> > On Thu, Aug 26, 2021 at 01:16:22PM -0500, Eric Sandeen wrote:
> > > 
> > > On 8/26/21 1:09 PM, Darrick J. Wong wrote:
> > > > On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
> > > 
> > > > > @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
> > > > >    	if (xfs_has_crc(mp))
> > > > >    		sb->s_flags |= SB_I_VERSION;
> > > > > -	if (xfs_has_dax_always(mp)) {
> > > > > +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
> > > > 
> > > > Er... can't this be done without burning another feature bit by:
> > > > 
> > > > 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
> > > > 				       !xfs_has_dax_never(mp))) {
> > > > 		...
> > > > 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
> > > > 	}
> > > 
> > > changing this conditional in this way will also fail dax=inode mounts on
> > > reflink-capable (i.e. default) filesystems, no?
> > 
> > Correct. My original patch tests fine, and still handles the reflink and dax
> > incompatibility. The new suggested logic is problematic.
> > -Bill
> 
> I think that both your proposed patch and Darrick's suggestion have this problem.
> 
> "mount -o dax=inode" makes your new xfs_has_dax_inode(mp) true, and in that
> conditional, if the filesystem has reflink enabled, mount fails:
> 
> # mkfs.xfs -f /dev/pmem0p1
> meta-data=/dev/pmem0p1           isize=512    agcount=4, agsize=4194304 blks
>          =                       sectsz=4096  attr=2, projid32bit=1
>          =                       crc=1        finobt=1, sparse=1, rmapbt=0
>          =                       reflink=1    bigtime=0 inobtcount=0
> data     =                       bsize=4096   blocks=16777216, imaxpct=25
>          =                       sunit=0      swidth=0 blks
> naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
> log      =internal log           bsize=4096   blocks=8192, version=2
>          =                       sectsz=4096  sunit=1 blks, lazy-count=1
> realtime =none                   extsz=4096   blocks=0, rtextents=0
> 
> # mount -o dax=inode /dev/pmem0p1 /mnt/test
> mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1,
>        missing codepage or helper program, or other error
> 
>        In some cases useful info is found in syslog - try
>        dmesg | tail or so.
> 
> # dmesg | tail -n 2
> [  192.691733] XFS (pmem0p1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
> [  192.700300] XFS (pmem0p1): DAX and reflink cannot be used together!
> 

So, the "DAX enabled" is a misnomer in this case. However the incompatibility of DAX and reflink is
reflected in the next message, and indeed the mount fails. Is it now a matter of fixing
the message output so as not to indicate "DAX enabled..."?

Thanks-
Bill


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-27 14:03         ` Bill O'Donnell
@ 2021-08-27 14:18           ` Eric Sandeen
  2021-08-27 14:25             ` Bill O'Donnell
  0 siblings, 1 reply; 12+ messages in thread
From: Eric Sandeen @ 2021-08-27 14:18 UTC (permalink / raw)
  To: Bill O'Donnell; +Cc: Darrick J. Wong, Bill O'Donnell, linux-xfs

On 8/27/21 9:03 AM, Bill O'Donnell wrote:
> On Thu, Aug 26, 2021 at 06:43:44PM -0500, Eric Sandeen wrote:
>> On 8/26/21 5:08 PM, Bill O'Donnell wrote:
>>> On Thu, Aug 26, 2021 at 01:16:22PM -0500, Eric Sandeen wrote:
>>>>
>>>> On 8/26/21 1:09 PM, Darrick J. Wong wrote:
>>>>> On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
>>>>
>>>>>> @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
>>>>>>     	if (xfs_has_crc(mp))
>>>>>>     		sb->s_flags |= SB_I_VERSION;
>>>>>> -	if (xfs_has_dax_always(mp)) {
>>>>>> +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
>>>>>
>>>>> Er... can't this be done without burning another feature bit by:
>>>>>
>>>>> 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
>>>>> 				       !xfs_has_dax_never(mp))) {
>>>>> 		...
>>>>> 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
>>>>> 	}
>>>>
>>>> changing this conditional in this way will also fail dax=inode mounts on
>>>> reflink-capable (i.e. default) filesystems, no?
>>>
>>> Correct. My original patch tests fine, and still handles the reflink and dax
>>> incompatibility. The new suggested logic is problematic.
>>> -Bill
>>
>> I think that both your proposed patch and Darrick's suggestion have this problem.
>>
>> "mount -o dax=inode" makes your new xfs_has_dax_inode(mp) true, and in that
>> conditional, if the filesystem has reflink enabled, mount fails:
>>
>> # mkfs.xfs -f /dev/pmem0p1
>> meta-data=/dev/pmem0p1           isize=512    agcount=4, agsize=4194304 blks
>>           =                       sectsz=4096  attr=2, projid32bit=1
>>           =                       crc=1        finobt=1, sparse=1, rmapbt=0
>>           =                       reflink=1    bigtime=0 inobtcount=0
>> data     =                       bsize=4096   blocks=16777216, imaxpct=25
>>           =                       sunit=0      swidth=0 blks
>> naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
>> log      =internal log           bsize=4096   blocks=8192, version=2
>>           =                       sectsz=4096  sunit=1 blks, lazy-count=1
>> realtime =none                   extsz=4096   blocks=0, rtextents=0
>>
>> # mount -o dax=inode /dev/pmem0p1 /mnt/test
>> mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1,
>>         missing codepage or helper program, or other error
>>
>>         In some cases useful info is found in syslog - try
>>         dmesg | tail or so.
>>
>> # dmesg | tail -n 2
>> [  192.691733] XFS (pmem0p1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
>> [  192.700300] XFS (pmem0p1): DAX and reflink cannot be used together!
>>
> 
> So, the "DAX enabled" is a misnomer in this case. However the incompatibility of DAX and reflink is
> reflected in the next message, and indeed the mount fails. Is it now a matter of fixing
> the message output so as not to indicate "DAX enabled..."?

The mount should not fail, and it does not fail prior to your change.

In the past, we did not allow any mixing of a reflink-capable
filesystem with dax in any way.  Now, with per-inode dax, dax-enabled inodes and
reflink-enabled inodes can exist on the same filesystem, you just cannot have an
inode which is both dax-enabled and reflinked at the same time.

-Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-27 14:18           ` Eric Sandeen
@ 2021-08-27 14:25             ` Bill O'Donnell
  2021-08-27 15:35               ` Eric Sandeen
  0 siblings, 1 reply; 12+ messages in thread
From: Bill O'Donnell @ 2021-08-27 14:25 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Darrick J. Wong, Bill O'Donnell, linux-xfs

On Fri, Aug 27, 2021 at 09:18:32AM -0500, Eric Sandeen wrote:
> On 8/27/21 9:03 AM, Bill O'Donnell wrote:
> > On Thu, Aug 26, 2021 at 06:43:44PM -0500, Eric Sandeen wrote:
> > > On 8/26/21 5:08 PM, Bill O'Donnell wrote:
> > > > On Thu, Aug 26, 2021 at 01:16:22PM -0500, Eric Sandeen wrote:
> > > > > 
> > > > > On 8/26/21 1:09 PM, Darrick J. Wong wrote:
> > > > > > On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
> > > > > 
> > > > > > > @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
> > > > > > >     	if (xfs_has_crc(mp))
> > > > > > >     		sb->s_flags |= SB_I_VERSION;
> > > > > > > -	if (xfs_has_dax_always(mp)) {
> > > > > > > +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
> > > > > > 
> > > > > > Er... can't this be done without burning another feature bit by:
> > > > > > 
> > > > > > 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
> > > > > > 				       !xfs_has_dax_never(mp))) {
> > > > > > 		...
> > > > > > 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
> > > > > > 	}
> > > > > 
> > > > > changing this conditional in this way will also fail dax=inode mounts on
> > > > > reflink-capable (i.e. default) filesystems, no?
> > > > 
> > > > Correct. My original patch tests fine, and still handles the reflink and dax
> > > > incompatibility. The new suggested logic is problematic.
> > > > -Bill
> > > 
> > > I think that both your proposed patch and Darrick's suggestion have this problem.
> > > 
> > > "mount -o dax=inode" makes your new xfs_has_dax_inode(mp) true, and in that
> > > conditional, if the filesystem has reflink enabled, mount fails:
> > > 
> > > # mkfs.xfs -f /dev/pmem0p1
> > > meta-data=/dev/pmem0p1           isize=512    agcount=4, agsize=4194304 blks
> > >           =                       sectsz=4096  attr=2, projid32bit=1
> > >           =                       crc=1        finobt=1, sparse=1, rmapbt=0
> > >           =                       reflink=1    bigtime=0 inobtcount=0
> > > data     =                       bsize=4096   blocks=16777216, imaxpct=25
> > >           =                       sunit=0      swidth=0 blks
> > > naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
> > > log      =internal log           bsize=4096   blocks=8192, version=2
> > >           =                       sectsz=4096  sunit=1 blks, lazy-count=1
> > > realtime =none                   extsz=4096   blocks=0, rtextents=0
> > > 
> > > # mount -o dax=inode /dev/pmem0p1 /mnt/test
> > > mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1,
> > >         missing codepage or helper program, or other error
> > > 
> > >         In some cases useful info is found in syslog - try
> > >         dmesg | tail or so.
> > > 
> > > # dmesg | tail -n 2
> > > [  192.691733] XFS (pmem0p1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
> > > [  192.700300] XFS (pmem0p1): DAX and reflink cannot be used together!
> > > 
> > 
> > So, the "DAX enabled" is a misnomer in this case. However the incompatibility of DAX and reflink is
> > reflected in the next message, and indeed the mount fails. Is it now a matter of fixing
> > the message output so as not to indicate "DAX enabled..."?
> 
> The mount should not fail, and it does not fail prior to your change.
> 
> In the past, we did not allow any mixing of a reflink-capable
> filesystem with dax in any way.  Now, with per-inode dax, dax-enabled inodes and
> reflink-enabled inodes can exist on the same filesystem, you just cannot have an
> inode which is both dax-enabled and reflinked at the same time.

Ah. I missed that nuance. I had thought the incompatibility was
absolute. :/

The manpage for mkfs.xfs may need updating for the inode mode
(unless mine is old):
----------------snip------------------
"Note:  the  filesystem DAX mount option ( -o dax ) is incom‐
patible  with  reflink-enabled  XFS  filesystems.   To   use
filesystem  DAX with XFS, specify the -m reflink=0 option to
mkfs.xfs to disable the reflink feature."
-------------------------------------
Thanks-
Bill


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-27 14:25             ` Bill O'Donnell
@ 2021-08-27 15:35               ` Eric Sandeen
  2021-08-27 15:41                 ` Bill O'Donnell
  0 siblings, 1 reply; 12+ messages in thread
From: Eric Sandeen @ 2021-08-27 15:35 UTC (permalink / raw)
  To: Bill O'Donnell; +Cc: Darrick J. Wong, Bill O'Donnell, linux-xfs

On 8/27/21 9:25 AM, Bill O'Donnell wrote:
> On Fri, Aug 27, 2021 at 09:18:32AM -0500, Eric Sandeen wrote:
>> On 8/27/21 9:03 AM, Bill O'Donnell wrote:
>>> On Thu, Aug 26, 2021 at 06:43:44PM -0500, Eric Sandeen wrote:
>>>> On 8/26/21 5:08 PM, Bill O'Donnell wrote:
>>>>> On Thu, Aug 26, 2021 at 01:16:22PM -0500, Eric Sandeen wrote:
>>>>>>
>>>>>> On 8/26/21 1:09 PM, Darrick J. Wong wrote:
>>>>>>> On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
>>>>>>
>>>>>>>> @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
>>>>>>>>      	if (xfs_has_crc(mp))
>>>>>>>>      		sb->s_flags |= SB_I_VERSION;
>>>>>>>> -	if (xfs_has_dax_always(mp)) {
>>>>>>>> +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
>>>>>>>
>>>>>>> Er... can't this be done without burning another feature bit by:
>>>>>>>
>>>>>>> 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
>>>>>>> 				       !xfs_has_dax_never(mp))) {
>>>>>>> 		...
>>>>>>> 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
>>>>>>> 	}
>>>>>>
>>>>>> changing this conditional in this way will also fail dax=inode mounts on
>>>>>> reflink-capable (i.e. default) filesystems, no?
>>>>>
>>>>> Correct. My original patch tests fine, and still handles the reflink and dax
>>>>> incompatibility. The new suggested logic is problematic.
>>>>> -Bill
>>>>
>>>> I think that both your proposed patch and Darrick's suggestion have this problem.
>>>>
>>>> "mount -o dax=inode" makes your new xfs_has_dax_inode(mp) true, and in that
>>>> conditional, if the filesystem has reflink enabled, mount fails:
>>>>
>>>> # mkfs.xfs -f /dev/pmem0p1
>>>> meta-data=/dev/pmem0p1           isize=512    agcount=4, agsize=4194304 blks
>>>>            =                       sectsz=4096  attr=2, projid32bit=1
>>>>            =                       crc=1        finobt=1, sparse=1, rmapbt=0
>>>>            =                       reflink=1    bigtime=0 inobtcount=0
>>>> data     =                       bsize=4096   blocks=16777216, imaxpct=25
>>>>            =                       sunit=0      swidth=0 blks
>>>> naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
>>>> log      =internal log           bsize=4096   blocks=8192, version=2
>>>>            =                       sectsz=4096  sunit=1 blks, lazy-count=1
>>>> realtime =none                   extsz=4096   blocks=0, rtextents=0
>>>>
>>>> # mount -o dax=inode /dev/pmem0p1 /mnt/test
>>>> mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1,
>>>>          missing codepage or helper program, or other error
>>>>
>>>>          In some cases useful info is found in syslog - try
>>>>          dmesg | tail or so.
>>>>
>>>> # dmesg | tail -n 2
>>>> [  192.691733] XFS (pmem0p1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
>>>> [  192.700300] XFS (pmem0p1): DAX and reflink cannot be used together!
>>>>
>>>
>>> So, the "DAX enabled" is a misnomer in this case. However the incompatibility of DAX and reflink is
>>> reflected in the next message, and indeed the mount fails. Is it now a matter of fixing
>>> the message output so as not to indicate "DAX enabled..."?
>>
>> The mount should not fail, and it does not fail prior to your change.
>>
>> In the past, we did not allow any mixing of a reflink-capable
>> filesystem with dax in any way.  Now, with per-inode dax, dax-enabled inodes and
>> reflink-enabled inodes can exist on the same filesystem, you just cannot have an
>> inode which is both dax-enabled and reflinked at the same time.
> 
> Ah. I missed that nuance. I had thought the incompatibility was
> absolute. :/
> 
> The manpage for mkfs.xfs may need updating for the inode mode
> (unless mine is old):
> ----------------snip------------------
> "Note:  the  filesystem DAX mount option ( -o dax ) is incom‐
> patible  with  reflink-enabled  XFS  filesystems.   To   use
> filesystem  DAX with XFS, specify the -m reflink=0 option to
> mkfs.xfs to disable the reflink feature."
> -------------------------------------

Hm, looks like the xfs(5) manpage got updated, but it seems mkfs.xfs(8) did not.

        dax=value
               Set  CPU  direct  access (DAX) behavior for the current filesystem.
               This mount option accepts the following values:

               "dax=inode"  DAX  will  be  enabled  only  on  regular  files  with
               FS_XFLAG_DAX applied.

               "dax=never"  DAX  will  not  be enabled for any files. FS_XFLAG_DAX
               will be ignored.

               "dax=always" DAX will be enabled for all regular files,  regardless
               of the FS_XFLAG_DAX state.

               If  no  option  is  used when mounting a filesystem stored on a DAX
               capable device, dax=inode will be used as default.

               For details regarding DAX behavior in kernel, please refer to  ker‐
               nel's documentation

I'll send a patch to fix up the mkfs manpage, thanks.

Thanks,
-Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-27 15:35               ` Eric Sandeen
@ 2021-08-27 15:41                 ` Bill O'Donnell
  0 siblings, 0 replies; 12+ messages in thread
From: Bill O'Donnell @ 2021-08-27 15:41 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Darrick J. Wong, Bill O'Donnell, linux-xfs

On Fri, Aug 27, 2021 at 10:35:56AM -0500, Eric Sandeen wrote:
> On 8/27/21 9:25 AM, Bill O'Donnell wrote:
> > On Fri, Aug 27, 2021 at 09:18:32AM -0500, Eric Sandeen wrote:
> > > On 8/27/21 9:03 AM, Bill O'Donnell wrote:
> > > > On Thu, Aug 26, 2021 at 06:43:44PM -0500, Eric Sandeen wrote:
> > > > > On 8/26/21 5:08 PM, Bill O'Donnell wrote:
> > > > > > On Thu, Aug 26, 2021 at 01:16:22PM -0500, Eric Sandeen wrote:
> > > > > > > 
> > > > > > > On 8/26/21 1:09 PM, Darrick J. Wong wrote:
> > > > > > > > On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
> > > > > > > 
> > > > > > > > > @@ -1584,7 +1586,7 @@ xfs_fs_fill_super(
> > > > > > > > >      	if (xfs_has_crc(mp))
> > > > > > > > >      		sb->s_flags |= SB_I_VERSION;
> > > > > > > > > -	if (xfs_has_dax_always(mp)) {
> > > > > > > > > +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
> > > > > > > > 
> > > > > > > > Er... can't this be done without burning another feature bit by:
> > > > > > > > 
> > > > > > > > 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
> > > > > > > > 				       !xfs_has_dax_never(mp))) {
> > > > > > > > 		...
> > > > > > > > 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
> > > > > > > > 	}
> > > > > > > 
> > > > > > > changing this conditional in this way will also fail dax=inode mounts on
> > > > > > > reflink-capable (i.e. default) filesystems, no?
> > > > > > 
> > > > > > Correct. My original patch tests fine, and still handles the reflink and dax
> > > > > > incompatibility. The new suggested logic is problematic.
> > > > > > -Bill
> > > > > 
> > > > > I think that both your proposed patch and Darrick's suggestion have this problem.
> > > > > 
> > > > > "mount -o dax=inode" makes your new xfs_has_dax_inode(mp) true, and in that
> > > > > conditional, if the filesystem has reflink enabled, mount fails:
> > > > > 
> > > > > # mkfs.xfs -f /dev/pmem0p1
> > > > > meta-data=/dev/pmem0p1           isize=512    agcount=4, agsize=4194304 blks
> > > > >            =                       sectsz=4096  attr=2, projid32bit=1
> > > > >            =                       crc=1        finobt=1, sparse=1, rmapbt=0
> > > > >            =                       reflink=1    bigtime=0 inobtcount=0
> > > > > data     =                       bsize=4096   blocks=16777216, imaxpct=25
> > > > >            =                       sunit=0      swidth=0 blks
> > > > > naming   =version 2              bsize=4096   ascii-ci=0, ftype=1
> > > > > log      =internal log           bsize=4096   blocks=8192, version=2
> > > > >            =                       sectsz=4096  sunit=1 blks, lazy-count=1
> > > > > realtime =none                   extsz=4096   blocks=0, rtextents=0
> > > > > 
> > > > > # mount -o dax=inode /dev/pmem0p1 /mnt/test
> > > > > mount: wrong fs type, bad option, bad superblock on /dev/pmem0p1,
> > > > >          missing codepage or helper program, or other error
> > > > > 
> > > > >          In some cases useful info is found in syslog - try
> > > > >          dmesg | tail or so.
> > > > > 
> > > > > # dmesg | tail -n 2
> > > > > [  192.691733] XFS (pmem0p1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
> > > > > [  192.700300] XFS (pmem0p1): DAX and reflink cannot be used together!
> > > > > 
> > > > 
> > > > So, the "DAX enabled" is a misnomer in this case. However the incompatibility of DAX and reflink is
> > > > reflected in the next message, and indeed the mount fails. Is it now a matter of fixing
> > > > the message output so as not to indicate "DAX enabled..."?
> > > 
> > > The mount should not fail, and it does not fail prior to your change.
> > > 
> > > In the past, we did not allow any mixing of a reflink-capable
> > > filesystem with dax in any way.  Now, with per-inode dax, dax-enabled inodes and
> > > reflink-enabled inodes can exist on the same filesystem, you just cannot have an
> > > inode which is both dax-enabled and reflinked at the same time.
> > 
> > Ah. I missed that nuance. I had thought the incompatibility was
> > absolute. :/
> > 
> > The manpage for mkfs.xfs may need updating for the inode mode
> > (unless mine is old):
> > ----------------snip------------------
> > "Note:  the  filesystem DAX mount option ( -o dax ) is incom‐
> > patible  with  reflink-enabled  XFS  filesystems.   To   use
> > filesystem  DAX with XFS, specify the -m reflink=0 option to
> > mkfs.xfs to disable the reflink feature."
> > -------------------------------------
> 
> Hm, looks like the xfs(5) manpage got updated, but it seems mkfs.xfs(8) did not.
> 
>        dax=value
>               Set  CPU  direct  access (DAX) behavior for the current filesystem.
>               This mount option accepts the following values:
> 
>               "dax=inode"  DAX  will  be  enabled  only  on  regular  files  with
>               FS_XFLAG_DAX applied.
> 
>               "dax=never"  DAX  will  not  be enabled for any files. FS_XFLAG_DAX
>               will be ignored.
> 
>               "dax=always" DAX will be enabled for all regular files,  regardless
>               of the FS_XFLAG_DAX state.
> 
>               If  no  option  is  used when mounting a filesystem stored on a DAX
>               capable device, dax=inode will be used as default.

The documentation here, https://www.kernel.org/doc/Documentation/filesystems/dax.txt
adds to the confusion.
   "-o dax"        is a legacy option which is an alias for "dax=always".
		    This may be removed in the future so "-o dax=always" is
		    the preferred method for specifying this behavior.



> 
>               For details regarding DAX behavior in kernel, please refer to  ker‐
>               nel's documentation
> 
> I'll send a patch to fix up the mkfs manpage, thanks.
> 
> Thanks,
> -Eric
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
  2021-08-26 18:09 ` Darrick J. Wong
  2021-08-26 18:14   ` Bill O'Donnell
  2021-08-26 18:16   ` Eric Sandeen
@ 2021-08-30 15:55   ` Bill O'Donnell
  2 siblings, 0 replies; 12+ messages in thread
From: Bill O'Donnell @ 2021-08-30 15:55 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Bill O'Donnell, linux-xfs

On Thu, Aug 26, 2021 at 11:09:47AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 26, 2021 at 12:30:12PM -0500, Bill O'Donnell wrote:
> > When dax-mode was tri-stated by adding dax=inode case, the EXPERIMENTAL
> > warning on mount was missed for the case. Add logic to handle the
> > warning similar to that of the 'dax=always' case.
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > ---
> >  fs/xfs/xfs_mount.h | 2 ++
> >  fs/xfs/xfs_super.c | 8 +++++---
> >  2 files changed, 7 insertions(+), 3 deletions(-)
... 

> > -	if (xfs_has_dax_always(mp)) {
> > +	if (xfs_has_dax_always(mp) || xfs_has_dax_inode(mp)) {
> 
> Er... can't this be done without burning another feature bit by:
> 
> 	if (xfs_has_dax_always(mp) || (!xfs_has_dax_always(mp) &&
> 				       !xfs_has_dax_never(mp))) {
> 		...
> 		xfs_warn(mp, "DAX IS EXPERIMENTAL");
> 	}

Not quite. This will be true at initialization.
-Bill


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-08-30 15:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-26 17:30 [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case Bill O'Donnell
2021-08-26 18:09 ` Darrick J. Wong
2021-08-26 18:14   ` Bill O'Donnell
2021-08-26 18:16   ` Eric Sandeen
2021-08-26 22:08     ` Bill O'Donnell
2021-08-26 23:43       ` Eric Sandeen
2021-08-27 14:03         ` Bill O'Donnell
2021-08-27 14:18           ` Eric Sandeen
2021-08-27 14:25             ` Bill O'Donnell
2021-08-27 15:35               ` Eric Sandeen
2021-08-27 15:41                 ` Bill O'Donnell
2021-08-30 15:55   ` Bill O'Donnell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).