All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ross Zwisler <ross.zwisler@linux.intel.com>
To: Eryu Guan <eguan@redhat.com>
Cc: Jan Kara <jack@suse.cz>, linux-nvdimm <linux-nvdimm@lists.01.org>,
	Amir Goldstein <amir73il@gmail.com>,
	Dave Chinner <david@fromorbit.com>,
	fstests <fstests@vger.kernel.org>,
	linux-xfs <linux-xfs@vger.kernel.org>
Subject: Re: [fstests PATCH v5 2/2] generic: add test for DAX MAP_SYNC support
Date: Thu, 7 Dec 2017 15:58:37 -0700	[thread overview]
Message-ID: <20171207225837.GA25831@linux.intel.com> (raw)
In-Reply-To: <20171207103657.GF2749@eguan.usersys.redhat.com>

On Thu, Dec 07, 2017 at 06:36:57PM +0800, Eryu Guan wrote:

> Now we have two _require rules to test log_writes dm target:
> _require_log_writes 	# _notrun explicitly when MOUNT_OPTIONS contains dax
> _require_log_writes_dax	# _notrun if log-writes target doesn't support dax
> 
> I think we can merge the two into one, i.e. extend _require_log_writes
> to check dax support status only when
> - MOUNT_OPTIONS contains dax, or
> - dax is given as a param explicitly, e.g. _require_log_writes dax
> 
> So old kernels that don't support dax log-writes still _notrun, and new
> kernels that have dax log-writes support could run all log-writes tests,
> like generic/455 and generic/457, in dax environment.
> 
> (I did a quick test with generic/45[57] on v4.15-rc2 kernel, 455 always
> fails due to md5sum mismatch, not sure where the problem is yet; 457 is
> _notrun, perhaps due to there's no dax support on reflink XFS.)

I looked in to generic/455 md5sum mismatch, and it is expected with the
current DAX support found in dm-log-writes.  The issue is that we snoop I/O,
but we *don't* snoop the data written by mmap().

For DAX mmaps, the sync calls msync()/fsync() don't cause writes to the block
driver of flushed page cache pages as they would in the page cache case.
Instead the user writes directly into the persistent memory, and all we see is
a flush call.  For us to properly handle mmap() writes we'll need to catch the
flush happening in dm-log-writes, iterate through the flushed data and copy it
into the dm-log-writes log.

I actually had this implemented in my initial version of the DAX support for
dm-log-writes, but by the time I was ready to merge the DAX flush path had
been removed from DM.  See
commit c3ca015fab6d ("dax: remove the pmem_dax_ops->flush abstraction")
for more info.

I can look at adding some level of flush support back into DM so we can handle
this case.  Until/unless that happens, though, I think we should continue to
make users specifically request DAX+dm-log-writes support that lacks mmap()
data verfication capabilities via _require_log_writes_dax.

Thank you for the feedback.  I'll post an updated patch that takes care of the
rest of your comments.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

WARNING: multiple messages have this Message-ID (diff)
From: Ross Zwisler <ross.zwisler@linux.intel.com>
To: Eryu Guan <eguan@redhat.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>,
	fstests <fstests@vger.kernel.org>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	linux-nvdimm <linux-nvdimm@lists.01.org>, Jan Kara <jack@suse.cz>,
	Dave Chinner <david@fromorbit.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Amir Goldstein <amir73il@gmail.com>
Subject: Re: [fstests PATCH v5 2/2] generic: add test for DAX MAP_SYNC support
Date: Thu, 7 Dec 2017 15:58:37 -0700	[thread overview]
Message-ID: <20171207225837.GA25831@linux.intel.com> (raw)
In-Reply-To: <20171207103657.GF2749@eguan.usersys.redhat.com>

On Thu, Dec 07, 2017 at 06:36:57PM +0800, Eryu Guan wrote:

> Now we have two _require rules to test log_writes dm target:
> _require_log_writes 	# _notrun explicitly when MOUNT_OPTIONS contains dax
> _require_log_writes_dax	# _notrun if log-writes target doesn't support dax
> 
> I think we can merge the two into one, i.e. extend _require_log_writes
> to check dax support status only when
> - MOUNT_OPTIONS contains dax, or
> - dax is given as a param explicitly, e.g. _require_log_writes dax
> 
> So old kernels that don't support dax log-writes still _notrun, and new
> kernels that have dax log-writes support could run all log-writes tests,
> like generic/455 and generic/457, in dax environment.
> 
> (I did a quick test with generic/45[57] on v4.15-rc2 kernel, 455 always
> fails due to md5sum mismatch, not sure where the problem is yet; 457 is
> _notrun, perhaps due to there's no dax support on reflink XFS.)

I looked in to generic/455 md5sum mismatch, and it is expected with the
current DAX support found in dm-log-writes.  The issue is that we snoop I/O,
but we *don't* snoop the data written by mmap().

For DAX mmaps, the sync calls msync()/fsync() don't cause writes to the block
driver of flushed page cache pages as they would in the page cache case.
Instead the user writes directly into the persistent memory, and all we see is
a flush call.  For us to properly handle mmap() writes we'll need to catch the
flush happening in dm-log-writes, iterate through the flushed data and copy it
into the dm-log-writes log.

I actually had this implemented in my initial version of the DAX support for
dm-log-writes, but by the time I was ready to merge the DAX flush path had
been removed from DM.  See
commit c3ca015fab6d ("dax: remove the pmem_dax_ops->flush abstraction")
for more info.

I can look at adding some level of flush support back into DM so we can handle
this case.  Until/unless that happens, though, I think we should continue to
make users specifically request DAX+dm-log-writes support that lacks mmap()
data verfication capabilities via _require_log_writes_dax.

Thank you for the feedback.  I'll post an updated patch that takes care of the
rest of your comments.

  reply	other threads:[~2017-12-07 22:54 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-06  0:37 [fstests PATCH v5 0/2] add test for DAX MAP_SYNC support Ross Zwisler
2017-12-06  0:37 ` Ross Zwisler
2017-12-06  0:37 ` [fstests PATCH v5 1/2] dm-log-writes: only replay log to marks that exist Ross Zwisler
2017-12-06  0:37   ` Ross Zwisler
2017-12-06 12:53   ` Amir Goldstein
2017-12-06 12:53     ` Amir Goldstein
2017-12-06  0:37 ` [fstests PATCH v5 2/2] generic: add test for DAX MAP_SYNC support Ross Zwisler
2017-12-06  0:37   ` Ross Zwisler
2017-12-06 13:41   ` Amir Goldstein
2017-12-06 13:41     ` Amir Goldstein
2017-12-07 10:36   ` Eryu Guan
2017-12-07 10:36     ` Eryu Guan
2017-12-07 22:58     ` Ross Zwisler [this message]
2017-12-07 22:58       ` Ross Zwisler
2017-12-07 23:19     ` [fstests PATCH v6 " Ross Zwisler
2017-12-07 23:19       ` Ross Zwisler
2017-12-08  6:36       ` Eryu Guan
2017-12-08  6:36         ` Eryu Guan
2017-12-08 17:47         ` Ross Zwisler
2017-12-08 17:47           ` Ross Zwisler
2017-12-10  6:15           ` Eryu Guan
2017-12-10  6:15             ` Eryu Guan

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=20171207225837.GA25831@linux.intel.com \
    --to=ross.zwisler@linux.intel.com \
    --cc=amir73il@gmail.com \
    --cc=david@fromorbit.com \
    --cc=eguan@redhat.com \
    --cc=fstests@vger.kernel.org \
    --cc=jack@suse.cz \
    --cc=linux-nvdimm@lists.01.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 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.