All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: Boaz Harrosh <boaz@plexistor.com>
Cc: linux-fsdevel@vger.kernel.org, willy@linux.intel.com,
	jack@suse.cz, xfs@oss.sgi.com
Subject: Re: [PATCH 8/8] xfs: add initial DAX support
Date: Wed, 25 Mar 2015 08:25:50 +1100	[thread overview]
Message-ID: <20150324212550.GU28621@dastard> (raw)
In-Reply-To: <55115E20.7090105@plexistor.com>

On Tue, Mar 24, 2015 at 02:52:48PM +0200, Boaz Harrosh wrote:
> On 03/24/2015 12:51 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > @@ -363,6 +365,10 @@ xfs_parseargs(
> >  			mp->m_flags |= XFS_MOUNT_DISCARD;
> >  		} else if (!strcmp(this_char, MNTOPT_NODISCARD)) {
> >  			mp->m_flags &= ~XFS_MOUNT_DISCARD;
> > +#ifdef CONFIG_FS_DAX
> > +		} else if (!strcmp(this_char, MNTOPT_DAX)) {
> > +			mp->m_flags |= XFS_MOUNT_DAX;
> 
> Hi
> 
> So what I see, (I might be wrong), is that once this flag is set here the
> fs (At above xfs_diflags_to_iflags() ) will start serving DAX inodes.

No, it won't, because...

> > @@ -1501,6 +1508,20 @@ xfs_fs_fill_super(
> >  	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
> >  		sb->s_flags |= MS_I_VERSION;
> >  
> > +	if (mp->m_flags & XFS_MOUNT_DAX) {
> > +		xfs_warn(mp,
> > +	"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> > +		if (sb->s_blocksize != PAGE_SIZE) {
> > +			xfs_alert(mp,
> > +		"Filesystem block size invalid for DAX Turning DAX off.");
> > +			mp->m_flags &= ~XFS_MOUNT_DAX;
> > +		} else if (!sb->s_bdev->bd_disk->fops->direct_access) {
> > +			xfs_alert(mp,
> > +		"Block device does not support DAX Turning DAX off.");
> > +			mp->m_flags &= ~XFS_MOUNT_DAX;
> > +		}
> > +	}

We run these tests and then clear the XFS_MOUNT_DAX flag later.

> If we agree about the s_flags MS_MOUNT_DAX  then we can define a
> 	if (MNTOPT_DAX)
> 		dax_enable_if_supported(sb);

No, I don't see any reason for a mount flag for this, because we
do not want to be stuck relying on a mount option forever. Once
This code has been shaken out, I fully intend DAX to be turned on
automatically for any device that supports, and for it to be turned
on and off on a per-inode basis through on-disk inode flags. i.e.
the mount option is really a temporary solution and I don't want to
code in any dependencies on DAX being mount wide existing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

WARNING: multiple messages have this Message-ID (diff)
From: Dave Chinner <david@fromorbit.com>
To: Boaz Harrosh <boaz@plexistor.com>
Cc: xfs@oss.sgi.com, linux-fsdevel@vger.kernel.org,
	willy@linux.intel.com, jack@suse.cz
Subject: Re: [PATCH 8/8] xfs: add initial DAX support
Date: Wed, 25 Mar 2015 08:25:50 +1100	[thread overview]
Message-ID: <20150324212550.GU28621@dastard> (raw)
In-Reply-To: <55115E20.7090105@plexistor.com>

On Tue, Mar 24, 2015 at 02:52:48PM +0200, Boaz Harrosh wrote:
> On 03/24/2015 12:51 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > @@ -363,6 +365,10 @@ xfs_parseargs(
> >  			mp->m_flags |= XFS_MOUNT_DISCARD;
> >  		} else if (!strcmp(this_char, MNTOPT_NODISCARD)) {
> >  			mp->m_flags &= ~XFS_MOUNT_DISCARD;
> > +#ifdef CONFIG_FS_DAX
> > +		} else if (!strcmp(this_char, MNTOPT_DAX)) {
> > +			mp->m_flags |= XFS_MOUNT_DAX;
> 
> Hi
> 
> So what I see, (I might be wrong), is that once this flag is set here the
> fs (At above xfs_diflags_to_iflags() ) will start serving DAX inodes.

No, it won't, because...

> > @@ -1501,6 +1508,20 @@ xfs_fs_fill_super(
> >  	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
> >  		sb->s_flags |= MS_I_VERSION;
> >  
> > +	if (mp->m_flags & XFS_MOUNT_DAX) {
> > +		xfs_warn(mp,
> > +	"DAX enabled. Warning: EXPERIMENTAL, use at your own risk");
> > +		if (sb->s_blocksize != PAGE_SIZE) {
> > +			xfs_alert(mp,
> > +		"Filesystem block size invalid for DAX Turning DAX off.");
> > +			mp->m_flags &= ~XFS_MOUNT_DAX;
> > +		} else if (!sb->s_bdev->bd_disk->fops->direct_access) {
> > +			xfs_alert(mp,
> > +		"Block device does not support DAX Turning DAX off.");
> > +			mp->m_flags &= ~XFS_MOUNT_DAX;
> > +		}
> > +	}

We run these tests and then clear the XFS_MOUNT_DAX flag later.

> If we agree about the s_flags MS_MOUNT_DAX  then we can define a
> 	if (MNTOPT_DAX)
> 		dax_enable_if_supported(sb);

No, I don't see any reason for a mount flag for this, because we
do not want to be stuck relying on a mount option forever. Once
This code has been shaken out, I fully intend DAX to be turned on
automatically for any device that supports, and for it to be turned
on and off on a per-inode basis through on-disk inode flags. i.e.
the mount option is really a temporary solution and I don't want to
code in any dependencies on DAX being mount wide existing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2015-03-24 21:25 UTC|newest]

Thread overview: 58+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-24 10:50 [PATCH 0/8 v2] xfs: DAX support Dave Chinner
2015-03-24 10:50 ` Dave Chinner
2015-03-24 10:50 ` [PATCH 1/8] xfs: mmap lock needs to be inside freeze protection Dave Chinner
2015-04-01 14:34   ` Jan Kara
2015-04-01 14:34     ` Jan Kara
2015-04-06 17:48   ` Brian Foster
2015-04-06 17:48     ` Brian Foster
2015-03-24 10:51 ` [PATCH 2/8] dax: don't abuse get_block mapping for endio callbacks Dave Chinner
2015-03-24 10:51   ` Dave Chinner
2015-04-01 14:53   ` Jan Kara
2015-04-01 14:53     ` Jan Kara
2015-03-24 10:51 ` [PATCH 3/8] dax: expose __dax_fault for filesystems with locking constraints Dave Chinner
2015-03-24 10:51   ` Dave Chinner
2015-04-01 15:07   ` Jan Kara
2015-04-01 15:07     ` Jan Kara
2015-03-24 10:51 ` [PATCH 4/8] xfs: add DAX block zeroing support Dave Chinner
2015-03-24 10:51   ` Dave Chinner
2015-04-06 17:48   ` Brian Foster
2015-04-06 17:48     ` Brian Foster
2015-03-24 10:51 ` [PATCH 5/8] xfs: add DAX file operations support Dave Chinner
2015-03-24 10:51   ` Dave Chinner
2015-03-24 12:08   ` Boaz Harrosh
2015-03-24 12:08     ` Boaz Harrosh
2015-03-24 12:24     ` Boaz Harrosh
2015-03-24 12:24       ` Boaz Harrosh
2015-03-24 21:17     ` Dave Chinner
2015-03-24 21:17       ` Dave Chinner
2015-03-25  8:47       ` Boaz Harrosh
2015-03-25  8:47         ` Boaz Harrosh
2015-04-06 17:49   ` Brian Foster
2015-04-06 17:49     ` Brian Foster
2015-04-16  8:29     ` Dave Chinner
2015-04-16  8:29       ` Dave Chinner
2015-04-16  9:33   ` Boaz Harrosh
2015-04-16  9:33     ` Boaz Harrosh
2015-04-16 11:47     ` Dave Chinner
2015-04-16 11:47       ` Dave Chinner
2015-03-24 10:51 ` [PATCH 6/8] xfs: add DAX truncate support Dave Chinner
2015-03-24 10:51   ` Dave Chinner
2015-04-06 17:49   ` Brian Foster
2015-04-06 17:49     ` Brian Foster
2015-03-24 10:51 ` [PATCH 7/8] xfs: add DAX IO path support Dave Chinner
2015-03-24 10:51   ` Dave Chinner
2015-04-06 17:49   ` Brian Foster
2015-04-06 17:49     ` Brian Foster
2015-04-16  8:54     ` Dave Chinner
2015-04-16  8:54       ` Dave Chinner
2015-03-24 10:51 ` [PATCH 8/8] xfs: add initial DAX support Dave Chinner
2015-03-24 10:51   ` Dave Chinner
2015-03-24 12:52   ` Boaz Harrosh
2015-03-24 12:52     ` Boaz Harrosh
2015-03-24 21:25     ` Dave Chinner [this message]
2015-03-24 21:25       ` Dave Chinner
2015-03-25  9:14       ` Boaz Harrosh
2015-03-25  9:14         ` Boaz Harrosh
2015-04-06 19:00   ` Brian Foster
2015-04-06 19:00     ` Brian Foster
  -- strict thread matches above, loose matches on Subject: below --
2015-05-28 23:45 [PATCH 0/8 v3] xfs: " Dave Chinner
2015-05-28 23:45 ` [PATCH 8/8] xfs: add initial " Dave Chinner

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=20150324212550.GU28621@dastard \
    --to=david@fromorbit.com \
    --cc=boaz@plexistor.com \
    --cc=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=willy@linux.intel.com \
    --cc=xfs@oss.sgi.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.