[{"id":1793464,"web_url":"http://patchwork.ozlabs.org/comment/1793464/","msgid":"<20171024212908.GC1611@linux.intel.com>","list_archive_url":null,"date":"2017-10-24T21:29:08","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":46514,"url":"http://patchwork.ozlabs.org/api/people/46514/","name":"Ross Zwisler","email":"ross.zwisler@linux.intel.com"},"content":"On Tue, Oct 24, 2017 at 05:24:14PM +0200, Jan Kara wrote:\n> From: Christoph Hellwig \n> \n> Return IOMAP_F_DIRTY from xfs_file_iomap_begin() when asked to prepare\n> blocks for writing and the inode is pinned, and has dirty fields other\n> than the timestamps. In __xfs_filemap_fault() we then detect this case\n> and call dax_finish_sync_fault() to make sure all metadata is committed,\n> and to insert the page table entry.\n> \n> Note that this will also dirty corresponding radix tree entry which is\n> what we want - fsync(2) will still provide data integrity guarantees for\n> applications not using userspace flushing. And applications using\n> userspace flushing can avoid calling fsync(2) and thus avoid the\n> performance overhead.\n> \n> [JK: Added VM_SYNC flag handling]\n> \n> Signed-off-by: Christoph Hellwig \n> Signed-off-by: Jan Kara \n\nI don't know enough about XFS dirty inode handling to be able to comment on\nthe changes in xfs_file_iomap_begin(), but the rest looks good.\n\nReviewed-by: Ross Zwisler ","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yM5xm02Stz9t2c\n\tfor ;\n\tWed, 25 Oct 2017 08:29:16 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751893AbdJXV3M (ORCPT );\n\tTue, 24 Oct 2017 17:29:12 -0400","from mga07.intel.com ([134.134.136.100]:47421 \"EHLO\n\tmga07.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751883AbdJXV3J (ORCPT );\n\tTue, 24 Oct 2017 17:29:09 -0400","from fmsmga002.fm.intel.com ([10.253.24.26])\n\tby orsmga105.jf.intel.com with ESMTP; 24 Oct 2017 14:29:09 -0700","from theros.lm.intel.com (HELO linux.intel.com) ([10.232.112.77])\n\tby fmsmga002.fm.intel.com with ESMTP; 24 Oct 2017 14:29:08 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.43,429,1503385200\"; d=\"scan'208\";a=\"1234806457\"","Date":"Tue, 24 Oct 2017 15:29:08 -0600","From":"Ross Zwisler ","To":"Jan Kara ","Cc":"Dan Williams ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4@vger.kernel.org, linux-nvdimm@lists.01.org,\n\tlinux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,\n\tlinux-api@vger.kernel.org, linux-mm@kvack.org,\n\tChristoph Hellwig ","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171024212908.GC1611@linux.intel.com>","Mail-Followup-To":"Ross Zwisler ,\n\tJan Kara , Dan Williams ,\n\tChristoph Hellwig , linux-ext4@vger.kernel.org,\n\tlinux-nvdimm@lists.01.org, linux-fsdevel@vger.kernel.org,\n\tlinux-xfs@vger.kernel.org, linux-api@vger.kernel.org,\n\tlinux-mm@kvack.org, Christoph Hellwig ","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171024152415.22864-18-jack@suse.cz>","User-Agent":"Mutt/1.9.1 (2017-09-22)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1793492,"web_url":"http://patchwork.ozlabs.org/comment/1793492/","msgid":"<20171024222322.GX3666@dastard>","list_archive_url":null,"date":"2017-10-24T22:23:22","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":421,"url":"http://patchwork.ozlabs.org/api/people/421/","name":"Dave Chinner","email":"david@fromorbit.com"},"content":"On Tue, Oct 24, 2017 at 05:24:14PM +0200, Jan Kara wrote:\n> From: Christoph Hellwig \n> \n> Return IOMAP_F_DIRTY from xfs_file_iomap_begin() when asked to prepare\n> blocks for writing and the inode is pinned, and has dirty fields other\n> than the timestamps.\n\nThat's \"fdatasync dirty\", not \"fsync dirty\".\n\nIOMAP_F_DIRTY needs a far better description of it's semantics than\n\"/* block mapping is not yet on persistent storage */\" so we know\nexactly what filesystems are supposed to be implementing here. I\nsuspect that what it really is meant to say is:\n\n/*\n * IOMAP_F_DIRTY indicates the inode has uncommitted metadata to\n * written data and requires fdatasync to commit to persistent storage.\n */\n\n[....]\n\n> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c\n> index f179bdf1644d..b43be199fbdf 100644\n> --- a/fs/xfs/xfs_iomap.c\n> +++ b/fs/xfs/xfs_iomap.c\n> @@ -33,6 +33,7 @@\n> #include \"xfs_error.h\"\n> #include \"xfs_trans.h\"\n> #include \"xfs_trans_space.h\"\n> +#include \"xfs_inode_item.h\"\n> #include \"xfs_iomap.h\"\n> #include \"xfs_trace.h\"\n> #include \"xfs_icache.h\"\n> @@ -1086,6 +1087,10 @@ xfs_file_iomap_begin(\n> \t\ttrace_xfs_iomap_found(ip, offset, length, 0, &imap);\n> \t}\n> \n> +\tif ((flags & IOMAP_WRITE) && xfs_ipincount(ip) &&\n> +\t (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))\n> +\t\tiomap->flags |= IOMAP_F_DIRTY;\n\nThis is the very definition of an inode that is \"fdatasync dirty\".\n\nHmmmm, shouldn't this also be set for read faults, too?\n\nCheers,\n\nDave.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yM78M52QTz9s7v\n\tfor ;\n\tWed, 25 Oct 2017 09:23:31 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751606AbdJXWX3 (ORCPT );\n\tTue, 24 Oct 2017 18:23:29 -0400","from ipmail06.adl6.internode.on.net ([150.101.137.145]:52750 \"EHLO\n\tipmail06.adl6.internode.on.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S1751522AbdJXWX2 (ORCPT\n\t);\n\tTue, 24 Oct 2017 18:23:28 -0400","from ppp59-167-129-252.static.internode.on.net (HELO dastard)\n\t([59.167.129.252]) by ipmail06.adl6.internode.on.net with ESMTP;\n\t25 Oct 2017 08:53:23 +1030","from dave by dastard with local (Exim 4.80)\n\t(envelope-from )\n\tid 1e77bW-000160-JM; Wed, 25 Oct 2017 09:23:22 +1100"],"Date":"Wed, 25 Oct 2017 09:23:22 +1100","From":"Dave Chinner ","To":"Jan Kara ","Cc":"Dan Williams ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4@vger.kernel.org, linux-nvdimm@lists.01.org,\n\tlinux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,\n\tlinux-api@vger.kernel.org, linux-mm@kvack.org,\n\tChristoph Hellwig ","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171024222322.GX3666@dastard>","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171024152415.22864-18-jack@suse.cz>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1794347,"web_url":"http://patchwork.ozlabs.org/comment/1794347/","msgid":"<20171026154804.GF31161@quack2.suse.cz>","list_archive_url":null,"date":"2017-10-26T15:48:04","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":363,"url":"http://patchwork.ozlabs.org/api/people/363/","name":"Jan Kara","email":"jack@suse.cz"},"content":"On Wed 25-10-17 09:23:22, Dave Chinner wrote:\n> On Tue, Oct 24, 2017 at 05:24:14PM +0200, Jan Kara wrote:\n> > From: Christoph Hellwig \n> > \n> > Return IOMAP_F_DIRTY from xfs_file_iomap_begin() when asked to prepare\n> > blocks for writing and the inode is pinned, and has dirty fields other\n> > than the timestamps.\n> \n> That's \"fdatasync dirty\", not \"fsync dirty\".\n\nCorrect.\n\n> IOMAP_F_DIRTY needs a far better description of it's semantics than\n> \"/* block mapping is not yet on persistent storage */\" so we know\n> exactly what filesystems are supposed to be implementing here. I\n> suspect that what it really is meant to say is:\n> \n> /*\n> * IOMAP_F_DIRTY indicates the inode has uncommitted metadata to\n> * written data and requires fdatasync to commit to persistent storage.\n> */\n\nI'll update the comment. Thanks!\n\n> [....]\n> \n> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c\n> > index f179bdf1644d..b43be199fbdf 100644\n> > --- a/fs/xfs/xfs_iomap.c\n> > +++ b/fs/xfs/xfs_iomap.c\n> > @@ -33,6 +33,7 @@\n> > #include \"xfs_error.h\"\n> > #include \"xfs_trans.h\"\n> > #include \"xfs_trans_space.h\"\n> > +#include \"xfs_inode_item.h\"\n> > #include \"xfs_iomap.h\"\n> > #include \"xfs_trace.h\"\n> > #include \"xfs_icache.h\"\n> > @@ -1086,6 +1087,10 @@ xfs_file_iomap_begin(\n> > \t\ttrace_xfs_iomap_found(ip, offset, length, 0, &imap);\n> > \t}\n> > \n> > +\tif ((flags & IOMAP_WRITE) && xfs_ipincount(ip) &&\n> > +\t (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))\n> > +\t\tiomap->flags |= IOMAP_F_DIRTY;\n> \n> This is the very definition of an inode that is \"fdatasync dirty\".\n> \n> Hmmmm, shouldn't this also be set for read faults, too?\n\nNo, read faults don't need to set IOMAP_F_DIRTY since user cannot write any\ndata to the page which he'd then like to be persistent. The only reason why\nI thought it could be useful for a while was that it would be nice to make\nMAP_SYNC mapping provide the guarantee that data you see now is the data\nyou'll see after a crash but we cannot provide that guarantee for RO\nmapping anyway if someone else has the page mapped as well. So I just\ndecided not to return IOMAP_F_DIRTY for read faults.\n\nBut now that I look at XFS implementation again, it misses handling\nof VM_FAULT_NEEDSYNC in xfs_filemap_pfn_mkwrite() (ext4 gets this right).\nI'll fix this by using __xfs_filemap_fault() for xfs_filemap_pfn_mkwrite()\nas well since it mostly duplicates it anyway... Thanks for inquiring!\n\n\t\t\t\t\t\t\t\tHonza","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yNBHH6CVgz9t5x\n\tfor ;\n\tFri, 27 Oct 2017 02:48:11 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932412AbdJZPsJ (ORCPT );\n\tThu, 26 Oct 2017 11:48:09 -0400","from mx2.suse.de ([195.135.220.15]:54936 \"EHLO mx2.suse.de\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S932137AbdJZPsI (ORCPT );\n\tThu, 26 Oct 2017 11:48:08 -0400","from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx2.suse.de (Postfix) with ESMTP id 67551ADE1;\n\tThu, 26 Oct 2017 15:48:06 +0000 (UTC)","by quack2.suse.cz (Postfix, from userid 1000)\n\tid DE52B1E0376; Thu, 26 Oct 2017 17:48:04 +0200 (CEST)"],"X-Virus-Scanned":"by amavisd-new at test-mx.suse.de","Date":"Thu, 26 Oct 2017 17:48:04 +0200","From":"Jan Kara ","To":"Dave Chinner ","Cc":"Jan Kara , Dan Williams ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4@vger.kernel.org, linux-nvdimm@lists.01.org,\n\tlinux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,\n\tlinux-api@vger.kernel.org, linux-mm@kvack.org,\n\tChristoph Hellwig ","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171026154804.GF31161@quack2.suse.cz>","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>\n\t<20171024222322.GX3666@dastard>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171024222322.GX3666@dastard>","User-Agent":"Mutt/1.5.24 (2015-08-30)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1794480,"web_url":"http://patchwork.ozlabs.org/comment/1794480/","msgid":"<20171026211611.GC3666@dastard>","list_archive_url":null,"date":"2017-10-26T21:16:11","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":421,"url":"http://patchwork.ozlabs.org/api/people/421/","name":"Dave Chinner","email":"david@fromorbit.com"},"content":"On Thu, Oct 26, 2017 at 05:48:04PM +0200, Jan Kara wrote:\n> On Wed 25-10-17 09:23:22, Dave Chinner wrote:\n> > On Tue, Oct 24, 2017 at 05:24:14PM +0200, Jan Kara wrote:\n> > > From: Christoph Hellwig \n> > > \n> > > Return IOMAP_F_DIRTY from xfs_file_iomap_begin() when asked to prepare\n> > > blocks for writing and the inode is pinned, and has dirty fields other\n> > > than the timestamps.\n> > \n> > That's \"fdatasync dirty\", not \"fsync dirty\".\n> \n> Correct.\n> \n> > IOMAP_F_DIRTY needs a far better description of it's semantics than\n> > \"/* block mapping is not yet on persistent storage */\" so we know\n> > exactly what filesystems are supposed to be implementing here. I\n> > suspect that what it really is meant to say is:\n> > \n> > /*\n> > * IOMAP_F_DIRTY indicates the inode has uncommitted metadata to\n> > * written data and requires fdatasync to commit to persistent storage.\n> > */\n> \n> I'll update the comment. Thanks!\n> \n> > [....]\n> > \n> > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c\n> > > index f179bdf1644d..b43be199fbdf 100644\n> > > --- a/fs/xfs/xfs_iomap.c\n> > > +++ b/fs/xfs/xfs_iomap.c\n> > > @@ -33,6 +33,7 @@\n> > > #include \"xfs_error.h\"\n> > > #include \"xfs_trans.h\"\n> > > #include \"xfs_trans_space.h\"\n> > > +#include \"xfs_inode_item.h\"\n> > > #include \"xfs_iomap.h\"\n> > > #include \"xfs_trace.h\"\n> > > #include \"xfs_icache.h\"\n> > > @@ -1086,6 +1087,10 @@ xfs_file_iomap_begin(\n> > > \t\ttrace_xfs_iomap_found(ip, offset, length, 0, &imap);\n> > > \t}\n> > > \n> > > +\tif ((flags & IOMAP_WRITE) && xfs_ipincount(ip) &&\n> > > +\t (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))\n> > > +\t\tiomap->flags |= IOMAP_F_DIRTY;\n> > \n> > This is the very definition of an inode that is \"fdatasync dirty\".\n> > \n> > Hmmmm, shouldn't this also be set for read faults, too?\n> \n> No, read faults don't need to set IOMAP_F_DIRTY since user cannot write any\n> data to the page which he'd then like to be persistent. The only reason why\n> I thought it could be useful for a while was that it would be nice to make\n> MAP_SYNC mapping provide the guarantee that data you see now is the data\n> you'll see after a crash\n\nIsn't that the entire point of MAP_SYNC? i.e. That when we return\nfrom a page fault, the app knows that the data and it's underlying\nextent is on persistent storage?\n\n> but we cannot provide that guarantee for RO\n> mapping anyway if someone else has the page mapped as well. So I just\n> decided not to return IOMAP_F_DIRTY for read faults.\n\nIf there are multiple MAP_SYNC mappings to the inode, I would have\nexpected that they all sync all of the data/metadata on every page\nfault, regardless of who dirtied the inode. An RO mapping doesn't\nmean the data/metadata on the inode can't change, it just means it\ncan't change through that mapping. Running fsync() to guarantee the\npersistence of that data/metadata doesn't actually changing any\ndata....\n\nIOWs, if read faults don't guarantee the mapped range has stable\nextents on a MAP_SYNC mapping, then I think MAP_SYNC is broken\nbecause it's not giving consistent guarantees to userspace. Yes, it\nworks fine when only one MAP_SYNC mapping is modifying the inode,\nbut the moment we have concurrent operations on the inode that\naren't MAP_SYNC or O_SYNC this goes out the window....\n\nCheers,\n\nDave.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yNKZG2bZCz9t5x\n\tfor ;\n\tFri, 27 Oct 2017 08:16:38 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S932377AbdJZVQf (ORCPT );\n\tThu, 26 Oct 2017 17:16:35 -0400","from ipmail06.adl2.internode.on.net ([150.101.137.129]:61169 \"EHLO\n\tipmail06.adl2.internode.on.net\" rhost-flags-OK-OK-OK-OK)\n\tby vger.kernel.org with ESMTP id S932345AbdJZVQe (ORCPT\n\t);\n\tThu, 26 Oct 2017 17:16:34 -0400","from ppp59-167-129-252.static.internode.on.net (HELO dastard)\n\t([59.167.129.252]) by ipmail06.adl2.internode.on.net with ESMTP;\n\t27 Oct 2017 07:46:13 +1030","from dave by dastard with local (Exim 4.80)\n\t(envelope-from )\n\tid 1e7pVb-00044a-U5; Fri, 27 Oct 2017 08:16:11 +1100"],"Date":"Fri, 27 Oct 2017 08:16:11 +1100","From":"Dave Chinner ","To":"Jan Kara ","Cc":"Dan Williams ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4@vger.kernel.org, linux-nvdimm@lists.01.org,\n\tlinux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,\n\tlinux-api@vger.kernel.org, linux-mm@kvack.org,\n\tChristoph Hellwig ","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171026211611.GC3666@dastard>","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>\n\t<20171024222322.GX3666@dastard>\n\t<20171026154804.GF31161@quack2.suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171026154804.GF31161@quack2.suse.cz>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1794642,"web_url":"http://patchwork.ozlabs.org/comment/1794642/","msgid":"<20171027064301.GC22931@lst.de>","list_archive_url":null,"date":"2017-10-27T06:43:01","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":82,"url":"http://patchwork.ozlabs.org/api/people/82/","name":"Christoph Hellwig","email":"hch@lst.de"},"content":"On Thu, Oct 26, 2017 at 05:48:04PM +0200, Jan Kara wrote:\n> But now that I look at XFS implementation again, it misses handling\n> of VM_FAULT_NEEDSYNC in xfs_filemap_pfn_mkwrite() (ext4 gets this right).\n> I'll fix this by using __xfs_filemap_fault() for xfs_filemap_pfn_mkwrite()\n> as well since it mostly duplicates it anyway... Thanks for inquiring!\n\nMy first patches move xfs_filemap_pfn_mkwrite to use __xfs_filemap_fault,\nbut that didn't work. Wish I'd remember why, though.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yNZ7w3LCsz9t39\n\tfor ;\n\tFri, 27 Oct 2017 17:43:08 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751719AbdJ0GnE (ORCPT );\n\tFri, 27 Oct 2017 02:43:04 -0400","from verein.lst.de ([213.95.11.211]:49553 \"EHLO newverein.lst.de\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751170AbdJ0GnD (ORCPT );\n\tFri, 27 Oct 2017 02:43:03 -0400","by newverein.lst.de (Postfix, from userid 2407)\n\tid ACAE468C4E; Fri, 27 Oct 2017 08:43:01 +0200 (CEST)"],"Date":"Fri, 27 Oct 2017 08:43:01 +0200","From":"Christoph Hellwig ","To":"Jan Kara ","Cc":"Dave Chinner ,\n\tDan Williams ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4@vger.kernel.org, linux-nvdimm@lists.01.org,\n\tlinux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,\n\tlinux-api@vger.kernel.org, linux-mm@kvack.org,\n\tChristoph Hellwig ","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171027064301.GC22931@lst.de>","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>\n\t<20171024222322.GX3666@dastard>\n\t<20171026154804.GF31161@quack2.suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171026154804.GF31161@quack2.suse.cz>","User-Agent":"Mutt/1.5.17 (2007-11-01)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1794725,"web_url":"http://patchwork.ozlabs.org/comment/1794725/","msgid":"<20171027091301.GG31161@quack2.suse.cz>","list_archive_url":null,"date":"2017-10-27T09:13:01","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":363,"url":"http://patchwork.ozlabs.org/api/people/363/","name":"Jan Kara","email":"jack@suse.cz"},"content":"On Fri 27-10-17 08:43:01, Christoph Hellwig wrote:\n> On Thu, Oct 26, 2017 at 05:48:04PM +0200, Jan Kara wrote:\n> > But now that I look at XFS implementation again, it misses handling\n> > of VM_FAULT_NEEDSYNC in xfs_filemap_pfn_mkwrite() (ext4 gets this right).\n> > I'll fix this by using __xfs_filemap_fault() for xfs_filemap_pfn_mkwrite()\n> > as well since it mostly duplicates it anyway... Thanks for inquiring!\n> \n> My first patches move xfs_filemap_pfn_mkwrite to use __xfs_filemap_fault,\n> but that didn't work. Wish I'd remember why, though.\n\nMaybe due to the additional check on IS_DAX(inode) in __xfs_filemap_fault()\nwhich could do the wrong thing if per-inode DAX flag is switched? Because\notherwise __xfs_filemap_fault(vmf, PE_SIZE_PTE, true) does exactly the same\nthing as xfs_filemap_pfn_mkwrite() did.\n\nIf we do care about per-inode DAX flag switching, I can just fixup\nxfs_filemap_pfn_mkwrite() but my understanding was that we ditched the idea\nat least until someone comes up with a reliable way to implement that...\n\n\t\t\t\t\t\t\tHonza","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yNdT00G5pz9t30\n\tfor ;\n\tFri, 27 Oct 2017 20:13:08 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752396AbdJ0JNF (ORCPT );\n\tFri, 27 Oct 2017 05:13:05 -0400","from mx2.suse.de ([195.135.220.15]:56207 \"EHLO mx2.suse.de\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752139AbdJ0JNE (ORCPT );\n\tFri, 27 Oct 2017 05:13:04 -0400","from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx2.suse.de (Postfix) with ESMTP id 6B85DAB02;\n\tFri, 27 Oct 2017 09:13:02 +0000 (UTC)","by quack2.suse.cz (Postfix, from userid 1000)\n\tid DFDD81E0611; Fri, 27 Oct 2017 11:13:01 +0200 (CEST)"],"X-Virus-Scanned":"by amavisd-new at test-mx.suse.de","Date":"Fri, 27 Oct 2017 11:13:01 +0200","From":"Jan Kara ","To":"Christoph Hellwig ","Cc":"Jan Kara , Dave Chinner ,\n\tDan Williams ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4@vger.kernel.org, linux-nvdimm@lists.01.org,\n\tlinux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,\n\tlinux-api@vger.kernel.org, linux-mm@kvack.org","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171027091301.GG31161@quack2.suse.cz>","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>\n\t<20171024222322.GX3666@dastard>\n\t<20171026154804.GF31161@quack2.suse.cz>\n\t<20171027064301.GC22931@lst.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171027064301.GC22931@lst.de>","User-Agent":"Mutt/1.5.24 (2015-08-30)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1794743,"web_url":"http://patchwork.ozlabs.org/comment/1794743/","msgid":"<20171027100834.GH31161@quack2.suse.cz>","list_archive_url":null,"date":"2017-10-27T10:08:34","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":363,"url":"http://patchwork.ozlabs.org/api/people/363/","name":"Jan Kara","email":"jack@suse.cz"},"content":"On Fri 27-10-17 08:16:11, Dave Chinner wrote:\n> On Thu, Oct 26, 2017 at 05:48:04PM +0200, Jan Kara wrote:\n> > > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c\n> > > > index f179bdf1644d..b43be199fbdf 100644\n> > > > --- a/fs/xfs/xfs_iomap.c\n> > > > +++ b/fs/xfs/xfs_iomap.c\n> > > > @@ -33,6 +33,7 @@\n> > > > #include \"xfs_error.h\"\n> > > > #include \"xfs_trans.h\"\n> > > > #include \"xfs_trans_space.h\"\n> > > > +#include \"xfs_inode_item.h\"\n> > > > #include \"xfs_iomap.h\"\n> > > > #include \"xfs_trace.h\"\n> > > > #include \"xfs_icache.h\"\n> > > > @@ -1086,6 +1087,10 @@ xfs_file_iomap_begin(\n> > > > \t\ttrace_xfs_iomap_found(ip, offset, length, 0, &imap);\n> > > > \t}\n> > > > \n> > > > +\tif ((flags & IOMAP_WRITE) && xfs_ipincount(ip) &&\n> > > > +\t (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))\n> > > > +\t\tiomap->flags |= IOMAP_F_DIRTY;\n> > > \n> > > This is the very definition of an inode that is \"fdatasync dirty\".\n> > > \n> > > Hmmmm, shouldn't this also be set for read faults, too?\n> > \n> > No, read faults don't need to set IOMAP_F_DIRTY since user cannot write any\n> > data to the page which he'd then like to be persistent. The only reason why\n> > I thought it could be useful for a while was that it would be nice to make\n> > MAP_SYNC mapping provide the guarantee that data you see now is the data\n> > you'll see after a crash\n> \n> Isn't that the entire point of MAP_SYNC? i.e. That when we return\n> from a page fault, the app knows that the data and it's underlying\n> extent is on persistent storage?\n> \n> > but we cannot provide that guarantee for RO\n> > mapping anyway if someone else has the page mapped as well. So I just\n> > decided not to return IOMAP_F_DIRTY for read faults.\n> \n> If there are multiple MAP_SYNC mappings to the inode, I would have\n> expected that they all sync all of the data/metadata on every page\n> fault, regardless of who dirtied the inode. An RO mapping doesn't\n\nWell, they all do sync regardless of who dirtied the inode on every *write*\nfault.\n\n> mean the data/metadata on the inode can't change, it just means it\n> can't change through that mapping. Running fsync() to guarantee the\n> persistence of that data/metadata doesn't actually changing any\n> data....\n> \n> IOWs, if read faults don't guarantee the mapped range has stable\n> extents on a MAP_SYNC mapping, then I think MAP_SYNC is broken\n> because it's not giving consistent guarantees to userspace. Yes, it\n> works fine when only one MAP_SYNC mapping is modifying the inode,\n> but the moment we have concurrent operations on the inode that\n> aren't MAP_SYNC or O_SYNC this goes out the window....\n\nMAP_SYNC as I've implemented it provides guarantees only for data the\nprocess has actually written. I agree with that and it was a conscious\ndecision. In my opinion that covers most usecases, provides reasonably\nsimple semantics (i.e., if you write data through MAP_SYNC mapping, you can\npersist it just using CPU instructions), and reasonable performance.\n\nNow you seem to suggest the semantics should be: \"Data you have read from or\nwritten to a MAP_SYNC mapping can be persisted using CPU instructions.\" And\nfrom implementation POV we can do that rather easily (just rip out the\nIOMAP_WRITE checks). But I'm unsure whether this additional guarantee would\nbe useful enough to justify the slowdown of read faults? I was not able to\ncome up with a good usecase and so I've decided for current semantics. What\ndo other people think?\n\nAnd now that I've spelled out exact semantics I don't think your comparison\nthat you can fsync() data you didn't write quite matches - with MAP_SYNC\nyou will have to at least read the data to be able to persist it and you\ndon't have that requirement for fsync() either...\n\n\t\t\t\t\t\t\t\tHonza","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yNfj66fVJz9t2Q\n\tfor ;\n\tFri, 27 Oct 2017 21:08:42 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752458AbdJ0KIl (ORCPT );\n\tFri, 27 Oct 2017 06:08:41 -0400","from mx2.suse.de ([195.135.220.15]:33855 \"EHLO mx2.suse.de\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751158AbdJ0KIh (ORCPT );\n\tFri, 27 Oct 2017 06:08:37 -0400","from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx2.suse.de (Postfix) with ESMTP id 0617DABED;\n\tFri, 27 Oct 2017 10:08:34 +0000 (UTC)","by quack2.suse.cz (Postfix, from userid 1000)\n\tid 2A1B11E11EB; Fri, 27 Oct 2017 12:08:34 +0200 (CEST)"],"X-Virus-Scanned":"by amavisd-new at test-mx.suse.de","Date":"Fri, 27 Oct 2017 12:08:34 +0200","From":"Jan Kara ","To":"Dave Chinner ","Cc":"Jan Kara , Dan Williams ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4@vger.kernel.org, linux-nvdimm@lists.01.org,\n\tlinux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,\n\tlinux-api@vger.kernel.org, linux-mm@kvack.org,\n\tChristoph Hellwig ","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171027100834.GH31161@quack2.suse.cz>","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>\n\t<20171024222322.GX3666@dastard>\n\t<20171026154804.GF31161@quack2.suse.cz>\n\t<20171026211611.GC3666@dastard>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171026211611.GC3666@dastard>","User-Agent":"Mutt/1.5.24 (2015-08-30)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1796716,"web_url":"http://patchwork.ozlabs.org/comment/1796716/","msgid":"<20171031151907.GB26128@quack2.suse.cz>","list_archive_url":null,"date":"2017-10-31T15:19:07","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":363,"url":"http://patchwork.ozlabs.org/api/people/363/","name":"Jan Kara","email":"jack@suse.cz"},"content":"On Fri 27-10-17 12:08:34, Jan Kara wrote:\n> On Fri 27-10-17 08:16:11, Dave Chinner wrote:\n> > On Thu, Oct 26, 2017 at 05:48:04PM +0200, Jan Kara wrote:\n> > > > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c\n> > > > > index f179bdf1644d..b43be199fbdf 100644\n> > > > > --- a/fs/xfs/xfs_iomap.c\n> > > > > +++ b/fs/xfs/xfs_iomap.c\n> > > > > @@ -33,6 +33,7 @@\n> > > > > #include \"xfs_error.h\"\n> > > > > #include \"xfs_trans.h\"\n> > > > > #include \"xfs_trans_space.h\"\n> > > > > +#include \"xfs_inode_item.h\"\n> > > > > #include \"xfs_iomap.h\"\n> > > > > #include \"xfs_trace.h\"\n> > > > > #include \"xfs_icache.h\"\n> > > > > @@ -1086,6 +1087,10 @@ xfs_file_iomap_begin(\n> > > > > \t\ttrace_xfs_iomap_found(ip, offset, length, 0, &imap);\n> > > > > \t}\n> > > > > \n> > > > > +\tif ((flags & IOMAP_WRITE) && xfs_ipincount(ip) &&\n> > > > > +\t (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))\n> > > > > +\t\tiomap->flags |= IOMAP_F_DIRTY;\n> > > > \n> > > > This is the very definition of an inode that is \"fdatasync dirty\".\n> > > > \n> > > > Hmmmm, shouldn't this also be set for read faults, too?\n> > > \n> > > No, read faults don't need to set IOMAP_F_DIRTY since user cannot write any\n> > > data to the page which he'd then like to be persistent. The only reason why\n> > > I thought it could be useful for a while was that it would be nice to make\n> > > MAP_SYNC mapping provide the guarantee that data you see now is the data\n> > > you'll see after a crash\n> > \n> > Isn't that the entire point of MAP_SYNC? i.e. That when we return\n> > from a page fault, the app knows that the data and it's underlying\n> > extent is on persistent storage?\n> > \n> > > but we cannot provide that guarantee for RO\n> > > mapping anyway if someone else has the page mapped as well. So I just\n> > > decided not to return IOMAP_F_DIRTY for read faults.\n> > \n> > If there are multiple MAP_SYNC mappings to the inode, I would have\n> > expected that they all sync all of the data/metadata on every page\n> > fault, regardless of who dirtied the inode. An RO mapping doesn't\n> \n> Well, they all do sync regardless of who dirtied the inode on every *write*\n> fault.\n> \n> > mean the data/metadata on the inode can't change, it just means it\n> > can't change through that mapping. Running fsync() to guarantee the\n> > persistence of that data/metadata doesn't actually changing any\n> > data....\n> > \n> > IOWs, if read faults don't guarantee the mapped range has stable\n> > extents on a MAP_SYNC mapping, then I think MAP_SYNC is broken\n> > because it's not giving consistent guarantees to userspace. Yes, it\n> > works fine when only one MAP_SYNC mapping is modifying the inode,\n> > but the moment we have concurrent operations on the inode that\n> > aren't MAP_SYNC or O_SYNC this goes out the window....\n> \n> MAP_SYNC as I've implemented it provides guarantees only for data the\n> process has actually written. I agree with that and it was a conscious\n> decision. In my opinion that covers most usecases, provides reasonably\n> simple semantics (i.e., if you write data through MAP_SYNC mapping, you can\n> persist it just using CPU instructions), and reasonable performance.\n> \n> Now you seem to suggest the semantics should be: \"Data you have read from or\n> written to a MAP_SYNC mapping can be persisted using CPU instructions.\" And\n> from implementation POV we can do that rather easily (just rip out the\n> IOMAP_WRITE checks). But I'm unsure whether this additional guarantee would\n> be useful enough to justify the slowdown of read faults? I was not able to\n> come up with a good usecase and so I've decided for current semantics. What\n> do other people think?\n\nNobody commented on this for couple of days so how do we proceed? I would\nprefer to go just with a guarantee for data written and we can always make\nthe guarantee stronger (i.e. apply it also for read data) when some user\ncomes with a good usecase?\n\n\t\t\t\t\t\t\t\tHonza","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yRFPg0F39z9ryv\n\tfor ;\n\tWed, 1 Nov 2017 02:19:19 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753195AbdJaPTL (ORCPT );\n\tTue, 31 Oct 2017 11:19:11 -0400","from mx2.suse.de ([195.135.220.15]:58630 \"EHLO mx2.suse.de\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751408AbdJaPTK (ORCPT );\n\tTue, 31 Oct 2017 11:19:10 -0400","from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx2.suse.de (Postfix) with ESMTP id 577F0ADF2;\n\tTue, 31 Oct 2017 15:19:08 +0000 (UTC)","by quack2.suse.cz (Postfix, from userid 1000)\n\tid BE0B01E1164; Tue, 31 Oct 2017 16:19:07 +0100 (CET)"],"X-Virus-Scanned":"by amavisd-new at test-mx.suse.de","Date":"Tue, 31 Oct 2017 16:19:07 +0100","From":"Jan Kara ","To":"Dave Chinner ","Cc":"Jan Kara , Dan Williams ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4@vger.kernel.org, linux-nvdimm@lists.01.org,\n\tlinux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,\n\tlinux-api@vger.kernel.org, linux-mm@kvack.org,\n\tChristoph Hellwig ","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171031151907.GB26128@quack2.suse.cz>","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>\n\t<20171024222322.GX3666@dastard>\n\t<20171026154804.GF31161@quack2.suse.cz>\n\t<20171026211611.GC3666@dastard>\n\t<20171027100834.GH31161@quack2.suse.cz>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20171027100834.GH31161@quack2.suse.cz>","User-Agent":"Mutt/1.5.24 (2015-08-30)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1796955,"web_url":"http://patchwork.ozlabs.org/comment/1796955/","msgid":"","list_archive_url":null,"date":"2017-10-31T21:50:01","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":347,"url":"http://patchwork.ozlabs.org/api/people/347/","name":"Dan Williams","email":"dan.j.williams@intel.com"},"content":"On Tue, Oct 31, 2017 at 8:19 AM, Jan Kara wrote:\n> On Fri 27-10-17 12:08:34, Jan Kara wrote:\n>> On Fri 27-10-17 08:16:11, Dave Chinner wrote:\n>> > On Thu, Oct 26, 2017 at 05:48:04PM +0200, Jan Kara wrote:\n>> > > > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c\n>> > > > > index f179bdf1644d..b43be199fbdf 100644\n>> > > > > --- a/fs/xfs/xfs_iomap.c\n>> > > > > +++ b/fs/xfs/xfs_iomap.c\n>> > > > > @@ -33,6 +33,7 @@\n>> > > > > #include \"xfs_error.h\"\n>> > > > > #include \"xfs_trans.h\"\n>> > > > > #include \"xfs_trans_space.h\"\n>> > > > > +#include \"xfs_inode_item.h\"\n>> > > > > #include \"xfs_iomap.h\"\n>> > > > > #include \"xfs_trace.h\"\n>> > > > > #include \"xfs_icache.h\"\n>> > > > > @@ -1086,6 +1087,10 @@ xfs_file_iomap_begin(\n>> > > > > trace_xfs_iomap_found(ip, offset, length, 0, &imap);\n>> > > > > }\n>> > > > >\n>> > > > > + if ((flags & IOMAP_WRITE) && xfs_ipincount(ip) &&\n>> > > > > + (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))\n>> > > > > + iomap->flags |= IOMAP_F_DIRTY;\n>> > > >\n>> > > > This is the very definition of an inode that is \"fdatasync dirty\".\n>> > > >\n>> > > > Hmmmm, shouldn't this also be set for read faults, too?\n>> > >\n>> > > No, read faults don't need to set IOMAP_F_DIRTY since user cannot write any\n>> > > data to the page which he'd then like to be persistent. The only reason why\n>> > > I thought it could be useful for a while was that it would be nice to make\n>> > > MAP_SYNC mapping provide the guarantee that data you see now is the data\n>> > > you'll see after a crash\n>> >\n>> > Isn't that the entire point of MAP_SYNC? i.e. That when we return\n>> > from a page fault, the app knows that the data and it's underlying\n>> > extent is on persistent storage?\n>> >\n>> > > but we cannot provide that guarantee for RO\n>> > > mapping anyway if someone else has the page mapped as well. So I just\n>> > > decided not to return IOMAP_F_DIRTY for read faults.\n>> >\n>> > If there are multiple MAP_SYNC mappings to the inode, I would have\n>> > expected that they all sync all of the data/metadata on every page\n>> > fault, regardless of who dirtied the inode. An RO mapping doesn't\n>>\n>> Well, they all do sync regardless of who dirtied the inode on every *write*\n>> fault.\n>>\n>> > mean the data/metadata on the inode can't change, it just means it\n>> > can't change through that mapping. Running fsync() to guarantee the\n>> > persistence of that data/metadata doesn't actually changing any\n>> > data....\n>> >\n>> > IOWs, if read faults don't guarantee the mapped range has stable\n>> > extents on a MAP_SYNC mapping, then I think MAP_SYNC is broken\n>> > because it's not giving consistent guarantees to userspace. Yes, it\n>> > works fine when only one MAP_SYNC mapping is modifying the inode,\n>> > but the moment we have concurrent operations on the inode that\n>> > aren't MAP_SYNC or O_SYNC this goes out the window....\n>>\n>> MAP_SYNC as I've implemented it provides guarantees only for data the\n>> process has actually written. I agree with that and it was a conscious\n>> decision. In my opinion that covers most usecases, provides reasonably\n>> simple semantics (i.e., if you write data through MAP_SYNC mapping, you can\n>> persist it just using CPU instructions), and reasonable performance.\n>>\n>> Now you seem to suggest the semantics should be: \"Data you have read from or\n>> written to a MAP_SYNC mapping can be persisted using CPU instructions.\" And\n>> from implementation POV we can do that rather easily (just rip out the\n>> IOMAP_WRITE checks). But I'm unsure whether this additional guarantee would\n>> be useful enough to justify the slowdown of read faults? I was not able to\n>> come up with a good usecase and so I've decided for current semantics. What\n>> do other people think?\n>\n> Nobody commented on this for couple of days so how do we proceed? I would\n> prefer to go just with a guarantee for data written and we can always make\n> the guarantee stronger (i.e. apply it also for read data) when some user\n> comes with a good usecase?\n\nI think it is easier to strengthen the guarantee than loosen it later\nespecially since it is not yet clear that we have a use case for the\nstronger semantic. At least the initial motivation for MAP_SYNC was\nfor writers.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel-com.20150623.gappssmtp.com\n\theader.i=@intel-com.20150623.gappssmtp.com\n\theader.b=\"u1QgAQiZ\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yRQ4Y6NxTz9t3x\n\tfor ;\n\tWed, 1 Nov 2017 08:50:05 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753876AbdJaVuE (ORCPT );\n\tTue, 31 Oct 2017 17:50:04 -0400","from mail-oi0-f65.google.com ([209.85.218.65]:50207 \"EHLO\n\tmail-oi0-f65.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753067AbdJaVuC (ORCPT\n\t); Tue, 31 Oct 2017 17:50:02 -0400","by mail-oi0-f65.google.com with SMTP id q4so701466oic.7\n\tfor ;\n\tTue, 31 Oct 2017 14:50:02 -0700 (PDT)","by 10.157.38.155 with HTTP; Tue, 31 Oct 2017 14:50:01 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=intel-com.20150623.gappssmtp.com; s=20150623;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc; bh=5q/rziWto24urQ0rNMy0Bf0Bx58TIqPtuEGuQH++/pM=;\n\tb=u1QgAQiZlbrKe2vVKljktdQ3+DiTHkvC+55CEZ32x/oT90ZzYR/hAZyYkUQVw8LET5\n\tyUY/6cUeX9MdCFuDZ7gVLe+o3HyG76HjLVtiFi70VsKdIoJeiygWyKBsd5dvDKZWQ6SR\n\t53B+BnMjaxIEkGA7o/UtBgZ+2RxBaoeW/FGhgT3bNdv3D9mgKmPpOotc75CJnq72RSiS\n\t8f2kSUNpvof3KHSPYNeK4pWNbfi6CNpd3KnhY54dwHX/QZV7xF+kohFtOri+aVl0e9VX\n\tK6EwN1mgq5kqSW2KkCFvGIOTKsMHmuKzaXbqmXEKjEOmmnv3199s3N0mv3ZnxWtRmk0J\n\tcQAw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc;\n\tbh=5q/rziWto24urQ0rNMy0Bf0Bx58TIqPtuEGuQH++/pM=;\n\tb=TyTTsSK8v41UYJ1GsgyHmiIFx6yW57EYZW6d4tbgBQAyuKU/MQ0Zm1MzYatkN+wt/E\n\tbO/4mg4dd7J/hV215qcTkktqnSQWKQvhzRjWeKZiHVoqmIjsQrHhZa2EtM1qoGjDf/QV\n\tb8/tsO5cskWHjAuxDPqOVvfh04hk/AIqFfB9TgFFWrb5mD5MyTuktiPHnvBA0l76wbUm\n\tu9qncSng6oIiCjL0SMrUVwUNIhzqBohp2w8Cct4tyqCe4fss04vwUROKHt67FL1ua8gn\n\tV6OlOg2cO/mwxz29UTeUdeS7/0D5QcYizSByUrgFMnVbfQifjcOktCOg6/FbymsS6qjq\n\t8x5w==","X-Gm-Message-State":"AMCzsaUb+OeWtVj5Op82+ltf0DsKUUqVpX6T0/LElj15XdAol5YsMuSr\n\tRdEsOmfqte3On3HuOWXVtxavDeqSdG8Bj9eWhUkGgA==","X-Google-Smtp-Source":"ABhQp+TzJf9NH7hU6ma1zg2kd6hnFzTxI4MPO2VwZikfzavPf2bZOUD5WD6lYazOZZCWQSs8rpjXVNyBqztxq9u5THE=","X-Received":"by 10.157.4.130 with SMTP id 2mr1812452otm.214.1509486601867;\n\tTue, 31 Oct 2017 14:50:01 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20171031151907.GB26128@quack2.suse.cz>","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>\n\t<20171024222322.GX3666@dastard>\n\t<20171026154804.GF31161@quack2.suse.cz>\n\t<20171026211611.GC3666@dastard>\n\t<20171027100834.GH31161@quack2.suse.cz>\n\t<20171031151907.GB26128@quack2.suse.cz>","From":"Dan Williams ","Date":"Tue, 31 Oct 2017 14:50:01 -0700","Message-ID":"","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","To":"Jan Kara ","Cc":"Dave Chinner ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4 ,\n\t\"linux-nvdimm@lists.01.org\" ,\n\tlinux-fsdevel ,\n\tlinux-xfs@vger.kernel.org, Linux API ,\n\tLinux MM , Christoph Hellwig ","Content-Type":"text/plain; charset=\"UTF-8\"","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1797079,"web_url":"http://patchwork.ozlabs.org/comment/1797079/","msgid":"<20171101034707.GA1662@linux.intel.com>","list_archive_url":null,"date":"2017-11-01T03:47:07","subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","submitter":{"id":46514,"url":"http://patchwork.ozlabs.org/api/people/46514/","name":"Ross Zwisler","email":"ross.zwisler@linux.intel.com"},"content":"On Tue, Oct 31, 2017 at 02:50:01PM -0700, Dan Williams wrote:\n> On Tue, Oct 31, 2017 at 8:19 AM, Jan Kara wrote:\n> > On Fri 27-10-17 12:08:34, Jan Kara wrote:\n> >> On Fri 27-10-17 08:16:11, Dave Chinner wrote:\n> >> > On Thu, Oct 26, 2017 at 05:48:04PM +0200, Jan Kara wrote:\n> >> > > > > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c\n> >> > > > > index f179bdf1644d..b43be199fbdf 100644\n> >> > > > > --- a/fs/xfs/xfs_iomap.c\n> >> > > > > +++ b/fs/xfs/xfs_iomap.c\n> >> > > > > @@ -33,6 +33,7 @@\n> >> > > > > #include \"xfs_error.h\"\n> >> > > > > #include \"xfs_trans.h\"\n> >> > > > > #include \"xfs_trans_space.h\"\n> >> > > > > +#include \"xfs_inode_item.h\"\n> >> > > > > #include \"xfs_iomap.h\"\n> >> > > > > #include \"xfs_trace.h\"\n> >> > > > > #include \"xfs_icache.h\"\n> >> > > > > @@ -1086,6 +1087,10 @@ xfs_file_iomap_begin(\n> >> > > > > trace_xfs_iomap_found(ip, offset, length, 0, &imap);\n> >> > > > > }\n> >> > > > >\n> >> > > > > + if ((flags & IOMAP_WRITE) && xfs_ipincount(ip) &&\n> >> > > > > + (ip->i_itemp->ili_fsync_fields & ~XFS_ILOG_TIMESTAMP))\n> >> > > > > + iomap->flags |= IOMAP_F_DIRTY;\n> >> > > >\n> >> > > > This is the very definition of an inode that is \"fdatasync dirty\".\n> >> > > >\n> >> > > > Hmmmm, shouldn't this also be set for read faults, too?\n> >> > >\n> >> > > No, read faults don't need to set IOMAP_F_DIRTY since user cannot write any\n> >> > > data to the page which he'd then like to be persistent. The only reason why\n> >> > > I thought it could be useful for a while was that it would be nice to make\n> >> > > MAP_SYNC mapping provide the guarantee that data you see now is the data\n> >> > > you'll see after a crash\n> >> >\n> >> > Isn't that the entire point of MAP_SYNC? i.e. That when we return\n> >> > from a page fault, the app knows that the data and it's underlying\n> >> > extent is on persistent storage?\n> >> >\n> >> > > but we cannot provide that guarantee for RO\n> >> > > mapping anyway if someone else has the page mapped as well. So I just\n> >> > > decided not to return IOMAP_F_DIRTY for read faults.\n> >> >\n> >> > If there are multiple MAP_SYNC mappings to the inode, I would have\n> >> > expected that they all sync all of the data/metadata on every page\n> >> > fault, regardless of who dirtied the inode. An RO mapping doesn't\n> >>\n> >> Well, they all do sync regardless of who dirtied the inode on every *write*\n> >> fault.\n> >>\n> >> > mean the data/metadata on the inode can't change, it just means it\n> >> > can't change through that mapping. Running fsync() to guarantee the\n> >> > persistence of that data/metadata doesn't actually changing any\n> >> > data....\n> >> >\n> >> > IOWs, if read faults don't guarantee the mapped range has stable\n> >> > extents on a MAP_SYNC mapping, then I think MAP_SYNC is broken\n> >> > because it's not giving consistent guarantees to userspace. Yes, it\n> >> > works fine when only one MAP_SYNC mapping is modifying the inode,\n> >> > but the moment we have concurrent operations on the inode that\n> >> > aren't MAP_SYNC or O_SYNC this goes out the window....\n> >>\n> >> MAP_SYNC as I've implemented it provides guarantees only for data the\n> >> process has actually written. I agree with that and it was a conscious\n> >> decision. In my opinion that covers most usecases, provides reasonably\n> >> simple semantics (i.e., if you write data through MAP_SYNC mapping, you can\n> >> persist it just using CPU instructions), and reasonable performance.\n> >>\n> >> Now you seem to suggest the semantics should be: \"Data you have read from or\n> >> written to a MAP_SYNC mapping can be persisted using CPU instructions.\" And\n> >> from implementation POV we can do that rather easily (just rip out the\n> >> IOMAP_WRITE checks). But I'm unsure whether this additional guarantee would\n> >> be useful enough to justify the slowdown of read faults? I was not able to\n> >> come up with a good usecase and so I've decided for current semantics. What\n> >> do other people think?\n> >\n> > Nobody commented on this for couple of days so how do we proceed? I would\n> > prefer to go just with a guarantee for data written and we can always make\n> > the guarantee stronger (i.e. apply it also for read data) when some user\n> > comes with a good usecase?\n> \n> I think it is easier to strengthen the guarantee than loosen it later\n> especially since it is not yet clear that we have a use case for the\n> stronger semantic. At least the initial motivation for MAP_SYNC was\n> for writers.\n\nI agree. It seems like all threads/processes in a given application need to\nuse MAP_SYNC consistently so they can be sure that data that is written (and\nthen possibly read) will be durable on media. I think what you have is a good\nstarting point, and we can adjust later if necessary.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-ext4-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yRZ0g2MTzz9t3r\n\tfor ;\n\tWed, 1 Nov 2017 14:47:15 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754291AbdKADrL (ORCPT );\n\tTue, 31 Oct 2017 23:47:11 -0400","from mga09.intel.com ([134.134.136.24]:21385 \"EHLO mga09.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1754278AbdKADrJ (ORCPT );\n\tTue, 31 Oct 2017 23:47:09 -0400","from fmsmga002.fm.intel.com ([10.253.24.26])\n\tby orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t31 Oct 2017 20:47:08 -0700","from theros.lm.intel.com (HELO linux.intel.com) ([10.232.112.77])\n\tby fmsmga002.fm.intel.com with ESMTP; 31 Oct 2017 20:47:07 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.44,326,1505804400\"; d=\"scan'208\";a=\"1238001808\"","Date":"Tue, 31 Oct 2017 21:47:07 -0600","From":"Ross Zwisler ","To":"Dan Williams ","Cc":"Jan Kara , Dave Chinner ,\n\tRoss Zwisler ,\n\tChristoph Hellwig ,\n\tlinux-ext4 ,\n\t\"linux-nvdimm@lists.01.org\" ,\n\tlinux-fsdevel ,\n\tlinux-xfs@vger.kernel.org, Linux API ,\n\tLinux MM , Christoph Hellwig ","Subject":"Re: [PATCH 17/17] xfs: support for synchronous DAX faults","Message-ID":"<20171101034707.GA1662@linux.intel.com>","Mail-Followup-To":"Ross Zwisler ,\n\tDan Williams , Jan Kara ,\n\tDave Chinner ,\n\tChristoph Hellwig ,\n\tlinux-ext4 ,\n\t\"linux-nvdimm@lists.01.org\" ,\n\tlinux-fsdevel ,\n\tlinux-xfs@vger.kernel.org, Linux API ,\n\tLinux MM , Christoph Hellwig ","References":"<20171024152415.22864-1-jack@suse.cz>\n\t<20171024152415.22864-18-jack@suse.cz>\n\t<20171024222322.GX3666@dastard>\n\t<20171026154804.GF31161@quack2.suse.cz>\n\t<20171026211611.GC3666@dastard>\n\t<20171027100834.GH31161@quack2.suse.cz>\n\t<20171031151907.GB26128@quack2.suse.cz>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.9.1 (2017-09-22)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}}]