linux-xfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Sandeen <sandeen@sandeen.net>
To: Bill O'Donnell <billodo@redhat.com>
Cc: "Darrick J. Wong" <djwong@kernel.org>,
	Bill O'Donnell <bodonnel@redhat.com>,
	linux-xfs@vger.kernel.org
Subject: Re: [PATCH] xfs: dax: facilitate EXPERIMENTAL warning for dax=inode case
Date: Fri, 27 Aug 2021 09:18:32 -0500	[thread overview]
Message-ID: <0876d0d8-557a-db32-f2c3-9d976cab6fad@sandeen.net> (raw)
In-Reply-To: <20210827140312.vzrwee5keck67w5p@redhat.com>

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

  reply	other threads:[~2021-08-27 14:18 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0876d0d8-557a-db32-f2c3-9d976cab6fad@sandeen.net \
    --to=sandeen@sandeen.net \
    --cc=billodo@redhat.com \
    --cc=bodonnel@redhat.com \
    --cc=djwong@kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).