xfs: don't fail dax mount w/ reflink if dax gets disabled

Commit Message

Eric Sandeen Sept. 5, 2018, 4:24 p.m. UTC

Today, we can get an interesting result when mounting a reflink filesystem
with -o dax on a device that doesn't support it:

XFS (sda1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
XFS (sda1): DAX unsupported by block device. Turning off DAX.
XFS (sda1): DAX and reflink cannot be used together!

<fail mount>

If we're willing to silently turn off DAX due to incompatibility with the
block device, it makes no sense to then fail the mount due to
incompatibility with the filesystem format.  So, skip this check if we
already decided to turn off DAX and proceed.

Signed-off-by: Eric Sandeen <[e-mail]>
---

Comments

Dave Chinner Sept. 5, 2018, 9:54 p.m. UTC | #1

On Wed, Sep 05, 2018 at 11:24:45AM -0500, Eric Sandeen wrote:
> Today, we can get an interesting result when mounting a reflink filesystem
> with -o dax on a device that doesn't support it:
> 
> XFS (sda1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
> XFS (sda1): DAX unsupported by block device. Turning off DAX.
> XFS (sda1): DAX and reflink cannot be used together!
> 
> <fail mount>
> 
> If we're willing to silently turn off DAX due to incompatibility with the
> block device, it makes no sense to then fail the mount due to
> incompatibility with the filesystem format.  So, skip this check if we
> already decided to turn off DAX and proceed.
> 
> Signed-off-by: Eric Sandeen <[e-mail]>
> ---
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 207ee30..c85c432 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1677,8 +1677,7 @@ struct proc_xfs_info {
>  			xfs_alert(mp,
>  			"DAX unsupported by block device. Turning off DAX.");
>  			mp->m_flags &= ~XFS_MOUNT_DAX;
> -		}
> -		if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> +		} else if (xfs_sb_version_hasreflink(&mp->m_sb)) {

Shouldn't this be:

		if ((mp->m_flags & XFS_MOUNT_DAX) &&
		    xfs_sb_version_hasreflink(&mp->m_sb)) {

>  			xfs_alert(mp,
>  		"DAX and reflink cannot be used together!");
>  			error = -EINVAL;

I.e. separate the reflink check form the initial mount option check
which may turn the mount option off?

I suspect we need to be more harsh are rejecting mounts with -o dax
on devices DAX isn't supported on. This mount option is going into
production systems - it's not just for "testing" as the comments all
claim. i Things will break in production systems if DAX isn't
enabled and they are expecting it to be enabled.

Combine that with block devices that can change their DAX support
without warning (see recent thread about dm dax behaviour), and
we've got a landmine that looks like "DAX works, I reboot, now DAX
doesn't work and I got no errors".

So perhaps this needs more thought w.r.t to rejection when -o dax is
used on non-dax devices.

Cheers,

Dave.

Eric Sandeen Sept. 5, 2018, 9:58 p.m. UTC | #2

On 9/5/18 4:54 PM, Dave Chinner wrote:
> On Wed, Sep 05, 2018 at 11:24:45AM -0500, Eric Sandeen wrote:
>> Today, we can get an interesting result when mounting a reflink filesystem
>> with -o dax on a device that doesn't support it:
>>
>> XFS (sda1): DAX enabled. Warning: EXPERIMENTAL, use at your own risk
>> XFS (sda1): DAX unsupported by block device. Turning off DAX.
>> XFS (sda1): DAX and reflink cannot be used together!
>>
>> <fail mount>
>>
>> If we're willing to silently turn off DAX due to incompatibility with the
>> block device, it makes no sense to then fail the mount due to
>> incompatibility with the filesystem format.  So, skip this check if we
>> already decided to turn off DAX and proceed.
>>
>> Signed-off-by: Eric Sandeen <[e-mail]>
>> ---
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index 207ee30..c85c432 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1677,8 +1677,7 @@ struct proc_xfs_info {
>>  			xfs_alert(mp,
>>  			"DAX unsupported by block device. Turning off DAX.");
>>  			mp->m_flags &= ~XFS_MOUNT_DAX;
>> -		}
>> -		if (xfs_sb_version_hasreflink(&mp->m_sb)) {
>> +		} else if (xfs_sb_version_hasreflink(&mp->m_sb)) {
> 
> Shouldn't this be:
> 
> 		if ((mp->m_flags & XFS_MOUNT_DAX) &&
> 		    xfs_sb_version_hasreflink(&mp->m_sb)) {
> 
>>  			xfs_alert(mp,
>>  		"DAX and reflink cannot be used together!");
>>  			error = -EINVAL;
> 
> I.e. separate the reflink check form the initial mount option check
> which may turn the mount option off?

Yes, I considered doing it that way too.  (either way works, right?)

> I suspect we need to be more harsh are rejecting mounts with -o dax
> on devices DAX isn't supported on. This mount option is going into
> production systems - it's not just for "testing" as the comments all
> claim. i Things will break in production systems if DAX isn't
> enabled and they are expecting it to be enabled.
> 
> Combine that with block devices that can change their DAX support
> without warning (see recent thread about dm dax behaviour), and
> we've got a landmine that looks like "DAX works, I reboot, now DAX
> doesn't work and I got no errors".
> 
> So perhaps this needs more thought w.r.t to rejection when -o dax is
> used on non-dax devices.

Yeah, I agree.  If we want to go the route of hard fail on -o dax on
unsupported devices, then we should make that change, and my patch
shouldn't be applied.

-Eric

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 207ee30..c85c432 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1677,8 +1677,7 @@  struct proc_xfs_info {
 			xfs_alert(mp,
 			"DAX unsupported by block device. Turning off DAX.");
 			mp->m_flags &= ~XFS_MOUNT_DAX;
-		}
-		if (xfs_sb_version_hasreflink(&mp->m_sb)) {
+		} else if (xfs_sb_version_hasreflink(&mp->m_sb)) {
 			xfs_alert(mp,
 		"DAX and reflink cannot be used together!");
 			error = -EINVAL;