[{"id":1764372,"web_url":"http://patchwork.ozlabs.org/comment/1764372/","msgid":"","list_archive_url":null,"date":"2017-09-06T20:55:31","subject":"Re: [PATCH 7/9] ext4: prevent data corruption with inline data + DAX","submitter":{"id":4514,"url":"http://patchwork.ozlabs.org/api/people/4514/","name":"Andreas Dilger","email":"adilger@dilger.ca"},"content":"On Sep 5, 2017, at 4:35 PM, Ross Zwisler wrote:\n> \n> If an inode has inline data it is currently prevented from using DAX by a\n> check in ext4_should_use_dax(). When the inode grows inline data via\n> ext4_create_inline_data() or removes its inline data via\n> ext4_destroy_inline_data_nolock(), the value of S_DAX can change.\n> \n> Currently these changes are unsafe because we don't hold off page faults\n> and I/O, write back dirty radix tree entries and invalidate all mappings.\n> This work is done in XFS via xfs_ioctl_setattr_dax_invalidate(), for\n> example. This unsafe transitioning of S_DAX could potentially lead to data\n> corruption.\n> \n> Fix this issue by preventing the DAX mount option from being used on\n> filesystems that were created to support inline data. Inline data is an\n> option given to mkfs.ext4.\n> \n> We prevent DAX from being used with inline data as opposed to trying to\n> safely manage the transition of S_DAX because of the locking complexities:\n> \n> 1) filemap_write_and_wait() eventually calls ext4_writepages(), which\n> acquires the sbi->s_journal_flag_rwsem. This lock ranks above the\n> jbdw_handle which is eventually taken by ext4_journal_start(). This\n> essentially means that the writeback has to happen outside of the context\n> of an active journal handle (outside of ext4_journal_start() to\n> ext4_journal_stop().)\n> \n> 2) To lock out page faults we take a write lock on the ei->i_mmap_sem, and\n> this lock again ranks above the jbd2_handle taken by ext4_journal_start().\n> So, as with the writeback code in 1) above we have to take ei->i_mmap_sem\n> outside of the context of an active journal handle.\n> \n> We are able to work around both of these restrictions and safely transition\n> S_DAX when we change the journaling mode, but for inline data it's much\n> harder because each of ext4_create_inline_data() and\n> ext4_destroy_inline_data_nolock() are passed in journal handles that have\n> already been started.\n> \n> To be able to safely writeback and invalidate our dirty inode data we'd\n> either need to uplevel the locking, writeback and invalidate into all the\n> callers of those two functions, or we'd need to stop our current journal\n> handle, do the appropriate locking, writeback and invalidate, unlock and\n> restart the journal handle.\n> \n> These both seem too complex, and I don't know if we have a valid use case\n> where we need to combine a filesystem with inline data and DAX, so just\n> prevent them from being used together.\n\nThe one reason I can see to use inline data + DAX is that inline data saves\nspace for very small files, even if the performance improvement is minimal.\nSince NVDIMMs are still relatively expensive, storing very small files and\ndirectories directly in the inode is probably worthwhile.\n\nThat said, there are still occasional bugs in the inline data code, so it\nmakes sense to ensure these two features are not enabled at the same time\nif they don't play well together.\n\n> Signed-off-by: Ross Zwisler \n> CC: stable@vger.kernel.org\n> ---\n> fs/ext4/inline.c | 10 ----------\n> fs/ext4/super.c | 5 +++++\n> 2 files changed, 5 insertions(+), 10 deletions(-)\n> \n> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c\n> index 28c5c3a..fd95019 100644\n> --- a/fs/ext4/inline.c\n> +++ b/fs/ext4/inline.c\n> @@ -302,11 +302,6 @@ static int ext4_create_inline_data(handle_t *handle,\n> \tEXT4_I(inode)->i_inline_size = len + EXT4_MIN_INLINE_DATA_SIZE;\n> \text4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);\n> \text4_set_inode_flag(inode, EXT4_INODE_INLINE_DATA);\n> -\t/*\n> -\t * Propagate changes to inode->i_flags as well - e.g. S_DAX may\n> -\t * get cleared\n> -\t */\n> -\text4_set_inode_flags(inode);\n\nWhat about other flags in the inode? It doesn't make sense to drop this\nentirely. The S_DAX flag shouldn't be set if the inode has the inline\ndata flag set, according to ext4_set_inode_flags().\n\n> \tget_bh(is.iloc.bh);\n> \terror = ext4_mark_iloc_dirty(handle, inode, &is.iloc);\n> \n> @@ -451,11 +446,6 @@ static int ext4_destroy_inline_data_nolock(handle_t *handle,\n> \t\t}\n> \t}\n> \text4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);\n> -\t/*\n> -\t * Propagate changes to inode->i_flags as well - e.g. S_DAX may\n> -\t * get set.\n> -\t */\n> -\text4_set_inode_flags(inode);\n\nSame?\n\n> \tget_bh(is.iloc.bh);\n> \terror = ext4_mark_iloc_dirty(handle, inode, &is.iloc);\n> diff --git a/fs/ext4/super.c b/fs/ext4/super.c\n> index d61a70e2..d549dfb 100644\n> --- a/fs/ext4/super.c\n> +++ b/fs/ext4/super.c\n> @@ -3686,6 +3686,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)\n> \t}\n> \n> \tif (sbi->s_mount_opt & EXT4_MOUNT_DAX) {\n> +\t\tif (ext4_has_feature_inline_data(sb)) {\n> +\t\t\text4_msg(sb, KERN_ERR, \"Cannot use DAX on a filesystem\"\n> +\t\t\t\t\t\" that may contain inline data\");\n> +\t\t\tgoto failed_mount;\n> +\t\t}\n\nWouldn't it be enough to just prevent modification of inodes that are stored\nin the inode? It should be OK to read such files. At a minimum that means\nthere should not be an error in case of read-only mounting. A better choice\nwould be to return an error only at runtime in case of open-for-write, or\nonly if the file is actually being written.\n\nCheers, Andreas","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=dilger-ca.20150623.gappssmtp.com\n\theader.i=@dilger-ca.20150623.gappssmtp.com\n\theader.b=\"CEmKd14d\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnbTN1TyBz9sRY\n\tfor ;\n\tThu, 7 Sep 2017 06:55:52 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753041AbdIFUzk (ORCPT );\n\tWed, 6 Sep 2017 16:55:40 -0400","from mail-it0-f68.google.com ([209.85.214.68]:38271 \"EHLO\n\tmail-it0-f68.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752693AbdIFUzi (ORCPT\n\t); Wed, 6 Sep 2017 16:55:38 -0400","by mail-it0-f68.google.com with SMTP id z81so18438itb.5\n\tfor ;\n\tWed, 06 Sep 2017 13:55:38 -0700 (PDT)","from cabot.adilger.ext (S0106a84e3fe4b223.cg.shawcable.net.\n\t[70.77.216.213]) by smtp.gmail.com with ESMTPSA id\n\tw17sm1220660ita.10.2017.09.06.13.55.34\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tWed, 06 Sep 2017 13:55:35 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=dilger-ca.20150623.gappssmtp.com; s=20150623;\n\th=from:message-id:mime-version:subject:date:in-reply-to:cc:to\n\t:references; bh=j9Bi2AwTYw98ILGHW0tzqrPagqvm2Js2mm5LMIEkvhw=;\n\tb=CEmKd14d0+N5n8hvpcfuBRhVqDycTnh1CAdMEosSL3AoUmEo1GoOWqn5yFoCugzXRx\n\th8SrsFLQpkj2wSB+arrNcgH9nCf2N5mV6iHX52C02FKqhcdNwW3l6d+62GrvTPkU2ZGy\n\tYMjRPV2j56w1poBWDz1YmLFeSF4Uw9hSwQr0GctHkzF4SscjOSDvKsNNG546orE5Ved1\n\tRruX6AgujEOjIdffLzqUdUNHZnsexRnRTPyAZYAyAW0Mvmgu+m2BrC9eHgIUeAXm8gyB\n\tCG5prctvSWDHVBhuMIZcZ9GG4lK04X4fsIGRjNnHZn7rcVAFDcD1jbBWX15QbYzAbYhx\n\tywaA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:message-id:mime-version:subject:date\n\t:in-reply-to:cc:to:references;\n\tbh=j9Bi2AwTYw98ILGHW0tzqrPagqvm2Js2mm5LMIEkvhw=;\n\tb=CThbgOTGmfzQ+CnqPeiuUT6PafZBGoZrN7Yz4TN2maxuTmwFyjq2nAJV9TdDUnMQou\n\t1OabRDA87e6UVhXO7BIFMekF5KFtFVSmE3nXVcjDrSbjTImy3cjbPCRwMyodGjfWPs86\n\tf+sbqPx+d6E7a5lABF2ilYrfmwZAtH/2KITh42uykPBWNh0TKyznh3s0e/B47qftyX15\n\taN/HprUOlbzZ4sveAV2AE1UBet6I+64eF63ZCRte5swiRKm6pcKk9fo1h0urJzksAgsi\n\t4VZ7W2Mw4dgCx53yiExmmaNuP1cyDJNX3/bzuQPNZZIDSOpAot5yN/xlrOYB+kHHOKyc\n\t9YZg==","X-Gm-Message-State":"AHPjjUikadj1IqBDdNJZdh/3vlm7T+5wehi5do6RWnv31oitDAiERm7X\n\tCHg07fISPaVTBnu6","X-Google-Smtp-Source":"ADKCNb4Dtgqb/ihyMyi8ROIaAZ09RoYRvT0Lo0f64nxp/jUFNOYjgG0kiV3GfODA+fj6DEAQ2lAgIw==","X-Received":"by 10.36.34.65 with SMTP id o62mr1054815ito.151.1504731337473;\n\tWed, 06 Sep 2017 13:55:37 -0700 (PDT)","From":"Andreas Dilger ","Message-Id":"","Content-Type":"multipart/signed;\n\tboundary=\"Apple-Mail=_FCD1889E-4F22-46C5-9BCC-A75FFB283D0A\";\n\tprotocol=\"application/pgp-signature\"; micalg=pgp-sha1","Mime-Version":"1.0 (Mac OS X Mail 10.3 \\(3273\\))","Subject":"Re: [PATCH 7/9] ext4: prevent data corruption with inline data + DAX","Date":"Wed, 6 Sep 2017 14:55:31 -0600","In-Reply-To":"<20170905223541.20594-8-ross.zwisler@linux.intel.com>","Cc":"Andrew Morton ,\n\tLKML ,\n\t\"Darrick J. Wong\" ,\n\tTheodore Ts'o , Christoph Hellwig ,\n\tDan Williams ,\n\tDave Chinner , Jan Kara ,\n\tExt4 Developers List ,\n\tlinux-nvdimm@lists.01.org, xfs ,\n\tstable@vger.kernel.org","To":"Ross Zwisler ","References":"<20170905223541.20594-1-ross.zwisler@linux.intel.com>\n\t<20170905223541.20594-8-ross.zwisler@linux.intel.com>","X-Mailer":"Apple Mail (2.3273)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}},{"id":1764427,"web_url":"http://patchwork.ozlabs.org/comment/1764427/","msgid":"<20170906231118.GA12679@linux.intel.com>","list_archive_url":null,"date":"2017-09-06T23:11:18","subject":"Re: [PATCH 7/9] ext4: prevent data corruption with inline data + DAX","submitter":{"id":46514,"url":"http://patchwork.ozlabs.org/api/people/46514/","name":"Ross Zwisler","email":"ross.zwisler@linux.intel.com"},"content":"On Wed, Sep 06, 2017 at 02:55:31PM -0600, Andreas Dilger wrote:\n> On Sep 5, 2017, at 4:35 PM, Ross Zwisler wrote:\n> > \n> > If an inode has inline data it is currently prevented from using DAX by a\n> > check in ext4_should_use_dax(). When the inode grows inline data via\n> > ext4_create_inline_data() or removes its inline data via\n> > ext4_destroy_inline_data_nolock(), the value of S_DAX can change.\n> > \n> > Currently these changes are unsafe because we don't hold off page faults\n> > and I/O, write back dirty radix tree entries and invalidate all mappings.\n> > This work is done in XFS via xfs_ioctl_setattr_dax_invalidate(), for\n> > example. This unsafe transitioning of S_DAX could potentially lead to data\n> > corruption.\n> > \n> > Fix this issue by preventing the DAX mount option from being used on\n> > filesystems that were created to support inline data. Inline data is an\n> > option given to mkfs.ext4.\n> > \n> > We prevent DAX from being used with inline data as opposed to trying to\n> > safely manage the transition of S_DAX because of the locking complexities:\n> > \n> > 1) filemap_write_and_wait() eventually calls ext4_writepages(), which\n> > acquires the sbi->s_journal_flag_rwsem. This lock ranks above the\n> > jbdw_handle which is eventually taken by ext4_journal_start(). This\n> > essentially means that the writeback has to happen outside of the context\n> > of an active journal handle (outside of ext4_journal_start() to\n> > ext4_journal_stop().)\n> > \n> > 2) To lock out page faults we take a write lock on the ei->i_mmap_sem, and\n> > this lock again ranks above the jbd2_handle taken by ext4_journal_start().\n> > So, as with the writeback code in 1) above we have to take ei->i_mmap_sem\n> > outside of the context of an active journal handle.\n> > \n> > We are able to work around both of these restrictions and safely transition\n> > S_DAX when we change the journaling mode, but for inline data it's much\n> > harder because each of ext4_create_inline_data() and\n> > ext4_destroy_inline_data_nolock() are passed in journal handles that have\n> > already been started.\n> > \n> > To be able to safely writeback and invalidate our dirty inode data we'd\n> > either need to uplevel the locking, writeback and invalidate into all the\n> > callers of those two functions, or we'd need to stop our current journal\n> > handle, do the appropriate locking, writeback and invalidate, unlock and\n> > restart the journal handle.\n> > \n> > These both seem too complex, and I don't know if we have a valid use case\n> > where we need to combine a filesystem with inline data and DAX, so just\n> > prevent them from being used together.\n> \n> The one reason I can see to use inline data + DAX is that inline data saves\n> space for very small files, even if the performance improvement is minimal.\n> Since NVDIMMs are still relatively expensive, storing very small files and\n> directories directly in the inode is probably worthwhile.\n> \n> That said, there are still occasional bugs in the inline data code, so it\n> makes sense to ensure these two features are not enabled at the same time\n> if they don't play well together.\n\nYep, unfortunately it's increasingly looking like we just can't ever safely\ntransition S_DAX at runtime:\n\nhttps://www.spinics.net/lists/linux-xfs/msg09859.html\n\nI think the current thought is to just avoid it completely until/unless we can\nfigure out a way to make it safe that is worthwhile.\n\n> > Signed-off-by: Ross Zwisler \n> > CC: stable@vger.kernel.org\n> > ---\n> > fs/ext4/inline.c | 10 ----------\n> > fs/ext4/super.c | 5 +++++\n> > 2 files changed, 5 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c\n> > index 28c5c3a..fd95019 100644\n> > --- a/fs/ext4/inline.c\n> > +++ b/fs/ext4/inline.c\n> > @@ -302,11 +302,6 @@ static int ext4_create_inline_data(handle_t *handle,\n> > \tEXT4_I(inode)->i_inline_size = len + EXT4_MIN_INLINE_DATA_SIZE;\n> > \text4_clear_inode_flag(inode, EXT4_INODE_EXTENTS);\n> > \text4_set_inode_flag(inode, EXT4_INODE_INLINE_DATA);\n> > -\t/*\n> > -\t * Propagate changes to inode->i_flags as well - e.g. S_DAX may\n> > -\t * get cleared\n> > -\t */\n> > -\text4_set_inode_flags(inode);\n> \n> What about other flags in the inode? It doesn't make sense to drop this\n> entirely. The S_DAX flag shouldn't be set if the inode has the inline\n> data flag set, according to ext4_set_inode_flags().\n\nAFAICT all the other flags in the inode are independent of\nEXT4_INODE_INLINE_DATA. The only one that could switch as a side-effect of\nEXT4_INODE_INLINE_DATA is S_DAX.\n\nThe lines I'm deleting were added explicitly for the side-effect transition of\nS_DAX (commit a3caa24b703794), so I don't think we still need them at all if\nwe prevent S_DAX from transitioning.\n\nDitto for the other deletion.\n\n> \n> > \tget_bh(is.iloc.bh);\n> > \terror = ext4_mark_iloc_dirty(handle, inode, &is.iloc);\n> > \n> > @@ -451,11 +446,6 @@ static int ext4_destroy_inline_data_nolock(handle_t *handle,\n> > \t\t}\n> > \t}\n> > \text4_clear_inode_flag(inode, EXT4_INODE_INLINE_DATA);\n> > -\t/*\n> > -\t * Propagate changes to inode->i_flags as well - e.g. S_DAX may\n> > -\t * get set.\n> > -\t */\n> > -\text4_set_inode_flags(inode);\n> \n> Same?\n> \n> > \tget_bh(is.iloc.bh);\n> > \terror = ext4_mark_iloc_dirty(handle, inode, &is.iloc);\n> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c\n> > index d61a70e2..d549dfb 100644\n> > --- a/fs/ext4/super.c\n> > +++ b/fs/ext4/super.c\n> > @@ -3686,6 +3686,11 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)\n> > \t}\n> > \n> > \tif (sbi->s_mount_opt & EXT4_MOUNT_DAX) {\n> > +\t\tif (ext4_has_feature_inline_data(sb)) {\n> > +\t\t\text4_msg(sb, KERN_ERR, \"Cannot use DAX on a filesystem\"\n> > +\t\t\t\t\t\" that may contain inline data\");\n> > +\t\t\tgoto failed_mount;\n> > +\t\t}\n> \n> Wouldn't it be enough to just prevent modification of inodes that are stored\n> in the inode? It should be OK to read such files. At a minimum that means\n> there should not be an error in case of read-only mounting. A better choice\n> would be to return an error only at runtime in case of open-for-write, or\n> only if the file is actually being written.\n\nHmm...yes? I think this *could* work - we'd have to prevent anything that\nwould make the inode switch from using inline data to using extents. This\nincludes writes (all writes? Or just ones that would trigger the\ntransition?), fallocate, etc.\n\nI think we could make this work, but the consequence would be that\napplications would receive I/O errors or errors for seemingly random syscalls,\nand they might have a hard time understanding why. \n\nIt seems cleaner and simpler to me to just disallow the combination of inline\ndata + DAX, and to print a descriptive message in dmesg? The combination of\nthe two features would end up being pretty unfriendly anyway, and the\nimplementation to make it safe would probably be more complex.","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 3xnfVF6mZrz9t2W\n\tfor ;\n\tThu, 7 Sep 2017 09:11:49 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1754233AbdIFXLV (ORCPT );\n\tWed, 6 Sep 2017 19:11:21 -0400","from mga04.intel.com ([192.55.52.120]:31254 \"EHLO mga04.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1752351AbdIFXLT (ORCPT );\n\tWed, 6 Sep 2017 19:11:19 -0400","from fmsmga005.fm.intel.com ([10.253.24.32])\n\tby fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t06 Sep 2017 16:11:19 -0700","from theros.lm.intel.com (HELO linux.intel.com) ([10.232.112.77])\n\tby fmsmga005.fm.intel.com with ESMTP; 06 Sep 2017 16:11:18 -0700"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,355,1500966000\"; d=\"scan'208\";a=\"148371149\"","Date":"Wed, 6 Sep 2017 17:11:18 -0600","From":"Ross Zwisler ","To":"Andreas Dilger ","Cc":"Ross Zwisler ,\n\tAndrew Morton ,\n\tLKML ,\n\t\"Darrick J. Wong\" ,\n\tTheodore Ts'o , Christoph Hellwig ,\n\tDan Williams ,\n\tDave Chinner , Jan Kara ,\n\tExt4 Developers List ,\n\tlinux-nvdimm@lists.01.org, xfs ,\n\tstable@vger.kernel.org","Subject":"Re: [PATCH 7/9] ext4: prevent data corruption with inline data + DAX","Message-ID":"<20170906231118.GA12679@linux.intel.com>","References":"<20170905223541.20594-1-ross.zwisler@linux.intel.com>\n\t<20170905223541.20594-8-ross.zwisler@linux.intel.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.8.3 (2017-05-23)","Sender":"linux-ext4-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-ext4@vger.kernel.org"}}]