From: Jan Kara <jack@suse.cz> To: Dan Williams <dan.j.williams@intel.com> Cc: Jan Kara <jack@suse.cz>, Matthew Wilcox <mawilcox@microsoft.com>, linux-nvdimm@lists.01.org, david@fromorbit.com, linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, Christoph Hellwig <hch@lst.de> Subject: Re: [PATCH v7 07/14] fs, dax: use page->mapping to warn if truncate collides with a busy page Date: Thu, 29 Mar 2018 18:02:15 +0200 [thread overview] Message-ID: <20180329160215.glnrmyunujcc4vwg@quack2.suse.cz> (raw) In-Reply-To: <152167306807.5268.8483232024444414342.stgit@dwillia2-desk3.amr.corp.intel.com> On Wed 21-03-18 15:57:48, Dan Williams wrote: > Catch cases where extent unmap operations encounter pages that are > pinned / busy. Typically this is pinned pages that are under active dma. > This warning is a canary for potential data corruption as truncated > blocks could be allocated to a new file while the device is still > performing i/o. > > Here is an example of a collision that this implementation catches: > > WARNING: CPU: 2 PID: 1286 at fs/dax.c:343 dax_disassociate_entry+0x55/0x80 > [..] > Call Trace: > __dax_invalidate_mapping_entry+0x6c/0xf0 > dax_delete_mapping_entry+0xf/0x20 > truncate_exceptional_pvec_entries.part.12+0x1af/0x200 > truncate_inode_pages_range+0x268/0x970 > ? tlb_gather_mmu+0x10/0x20 > ? up_write+0x1c/0x40 > ? unmap_mapping_range+0x73/0x140 > xfs_free_file_space+0x1b6/0x5b0 [xfs] > ? xfs_file_fallocate+0x7f/0x320 [xfs] > ? down_write_nested+0x40/0x70 > ? xfs_ilock+0x21d/0x2f0 [xfs] > xfs_file_fallocate+0x162/0x320 [xfs] > ? rcu_read_lock_sched_held+0x3f/0x70 > ? rcu_sync_lockdep_assert+0x2a/0x50 > ? __sb_start_write+0xd0/0x1b0 > ? vfs_fallocate+0x20c/0x270 > vfs_fallocate+0x154/0x270 > SyS_fallocate+0x43/0x80 > entry_SYSCALL_64_fastpath+0x1f/0x96 > > Cc: Jeff Moyer <jmoyer@redhat.com> > Cc: Matthew Wilcox <mawilcox@microsoft.com> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com> > Reviewed-by: Jan Kara <jack@suse.cz> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Two comments when looking at this now: > +#define for_each_entry_pfn(entry, pfn, end_pfn) \ > + for (pfn = dax_radix_pfn(entry), \ > + end_pfn = pfn + dax_entry_size(entry) / PAGE_SIZE; \ > + pfn < end_pfn; \ > + pfn++) Why don't you declare 'end_pfn' inside the for() block? That way you don't have to pass the variable as an argument to for_each_entry_pfn(). It's not like you need end_pfn anywhere in the loop body, you just use it to cache loop termination index. > @@ -547,6 +599,10 @@ static void *dax_insert_mapping_entry(struct address_space *mapping, > > spin_lock_irq(&mapping->tree_lock); > new_entry = dax_radix_locked_entry(pfn, flags); > + if (dax_entry_size(entry) != dax_entry_size(new_entry)) { > + dax_disassociate_entry(entry, mapping, false); > + dax_associate_entry(new_entry, mapping); > + } I find it quite tricky that in case we pass zero page / empty entry into dax_[dis]associate_entry(), it will not do anything because dax_entry_size() will return 0. Can we add an explicit check into dax_[dis]associate_entry() or at least a comment there? Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR _______________________________________________ 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: Jan Kara <jack@suse.cz> To: Dan Williams <dan.j.williams@intel.com> Cc: linux-nvdimm@lists.01.org, Jeff Moyer <jmoyer@redhat.com>, Matthew Wilcox <mawilcox@microsoft.com>, Ross Zwisler <ross.zwisler@linux.intel.com>, Jan Kara <jack@suse.cz>, Christoph Hellwig <hch@lst.de>, david@fromorbit.com, linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v7 07/14] fs, dax: use page->mapping to warn if truncate collides with a busy page Date: Thu, 29 Mar 2018 18:02:15 +0200 [thread overview] Message-ID: <20180329160215.glnrmyunujcc4vwg@quack2.suse.cz> (raw) In-Reply-To: <152167306807.5268.8483232024444414342.stgit@dwillia2-desk3.amr.corp.intel.com> On Wed 21-03-18 15:57:48, Dan Williams wrote: > Catch cases where extent unmap operations encounter pages that are > pinned / busy. Typically this is pinned pages that are under active dma. > This warning is a canary for potential data corruption as truncated > blocks could be allocated to a new file while the device is still > performing i/o. > > Here is an example of a collision that this implementation catches: > > WARNING: CPU: 2 PID: 1286 at fs/dax.c:343 dax_disassociate_entry+0x55/0x80 > [..] > Call Trace: > __dax_invalidate_mapping_entry+0x6c/0xf0 > dax_delete_mapping_entry+0xf/0x20 > truncate_exceptional_pvec_entries.part.12+0x1af/0x200 > truncate_inode_pages_range+0x268/0x970 > ? tlb_gather_mmu+0x10/0x20 > ? up_write+0x1c/0x40 > ? unmap_mapping_range+0x73/0x140 > xfs_free_file_space+0x1b6/0x5b0 [xfs] > ? xfs_file_fallocate+0x7f/0x320 [xfs] > ? down_write_nested+0x40/0x70 > ? xfs_ilock+0x21d/0x2f0 [xfs] > xfs_file_fallocate+0x162/0x320 [xfs] > ? rcu_read_lock_sched_held+0x3f/0x70 > ? rcu_sync_lockdep_assert+0x2a/0x50 > ? __sb_start_write+0xd0/0x1b0 > ? vfs_fallocate+0x20c/0x270 > vfs_fallocate+0x154/0x270 > SyS_fallocate+0x43/0x80 > entry_SYSCALL_64_fastpath+0x1f/0x96 > > Cc: Jeff Moyer <jmoyer@redhat.com> > Cc: Matthew Wilcox <mawilcox@microsoft.com> > Cc: Ross Zwisler <ross.zwisler@linux.intel.com> > Reviewed-by: Jan Kara <jack@suse.cz> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Dan Williams <dan.j.williams@intel.com> Two comments when looking at this now: > +#define for_each_entry_pfn(entry, pfn, end_pfn) \ > + for (pfn = dax_radix_pfn(entry), \ > + end_pfn = pfn + dax_entry_size(entry) / PAGE_SIZE; \ > + pfn < end_pfn; \ > + pfn++) Why don't you declare 'end_pfn' inside the for() block? That way you don't have to pass the variable as an argument to for_each_entry_pfn(). It's not like you need end_pfn anywhere in the loop body, you just use it to cache loop termination index. > @@ -547,6 +599,10 @@ static void *dax_insert_mapping_entry(struct address_space *mapping, > > spin_lock_irq(&mapping->tree_lock); > new_entry = dax_radix_locked_entry(pfn, flags); > + if (dax_entry_size(entry) != dax_entry_size(new_entry)) { > + dax_disassociate_entry(entry, mapping, false); > + dax_associate_entry(new_entry, mapping); > + } I find it quite tricky that in case we pass zero page / empty entry into dax_[dis]associate_entry(), it will not do anything because dax_entry_size() will return 0. Can we add an explicit check into dax_[dis]associate_entry() or at least a comment there? Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
next prev parent reply other threads:[~2018-03-29 15:55 UTC|newest] Thread overview: 90+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-21 22:57 [PATCH v7 00/14] dax: fix dma vs truncate/hole-punch Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` [PATCH v7 01/14] dax: store pfns in the radix Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` [PATCH v7 02/14] fs, dax: prepare for dax-specific address_space_operations Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-29 15:28 ` Jan Kara 2018-03-29 15:28 ` Jan Kara 2018-03-21 22:57 ` [PATCH v7 03/14] block, dax: remove dead code in blkdev_writepages() Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-29 15:29 ` Jan Kara 2018-03-29 15:29 ` Jan Kara 2018-03-21 22:57 ` [PATCH v7 04/14] xfs, dax: introduce xfs_dax_aops Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-29 15:30 ` Jan Kara 2018-03-29 15:30 ` Jan Kara 2018-03-21 22:57 ` [PATCH v7 05/14] ext4, dax: introduce ext4_dax_aops Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-29 15:40 ` Jan Kara 2018-03-29 15:40 ` Jan Kara 2018-03-29 15:40 ` Jan Kara 2018-03-29 18:09 ` Christoph Hellwig 2018-03-29 18:09 ` Christoph Hellwig 2018-03-29 18:09 ` Christoph Hellwig 2018-03-29 22:47 ` Dan Williams 2018-03-29 22:47 ` Dan Williams 2018-03-29 22:47 ` Dan Williams 2018-03-21 22:57 ` [PATCH v7 06/14] ext2, dax: introduce ext2_dax_aops Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-29 15:44 ` Jan Kara 2018-03-29 15:44 ` Jan Kara 2018-03-21 22:57 ` [PATCH v7 07/14] fs, dax: use page->mapping to warn if truncate collides with a busy page Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-29 16:02 ` Jan Kara [this message] 2018-03-29 16:02 ` Jan Kara 2018-03-29 19:02 ` Dan Williams 2018-03-29 19:02 ` Dan Williams 2018-03-29 23:02 ` Dan Williams 2018-03-29 23:02 ` Dan Williams 2018-03-30 8:22 ` Jan Kara 2018-03-30 8:22 ` Jan Kara 2018-03-21 22:57 ` [PATCH v7 08/14] mm, dax: enable filesystems to trigger dev_pagemap ->page_free callbacks Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-29 16:36 ` Jan Kara 2018-03-29 16:36 ` Jan Kara 2018-03-29 16:36 ` Jan Kara 2018-03-21 22:57 ` [PATCH v7 09/14] mm, dev_pagemap: introduce CONFIG_DEV_PAGEMAP_OPS Dan Williams 2018-03-21 22:57 ` Dan Williams 2018-03-22 7:49 ` Christoph Hellwig 2018-03-22 7:49 ` Christoph Hellwig 2018-03-30 10:37 ` Jan Kara 2018-03-30 10:37 ` Jan Kara 2018-03-30 10:37 ` Jan Kara 2018-03-21 22:58 ` [PATCH v7 10/14] memremap: mark devm_memremap_pages() EXPORT_SYMBOL_GPL Dan Williams 2018-03-21 22:58 ` Dan Williams 2018-03-22 7:51 ` Christoph Hellwig 2018-03-22 7:51 ` Christoph Hellwig 2018-03-21 22:58 ` [PATCH v7 11/14] mm, fs, dax: handle layout changes to pinned dax mappings Dan Williams 2018-03-21 22:58 ` Dan Williams 2018-03-21 22:58 ` Dan Williams 2018-03-21 22:58 ` [PATCH v7 12/14] xfs: prepare xfs_break_layouts() to be called with XFS_MMAPLOCK_EXCL Dan Williams 2018-03-21 22:58 ` Dan Williams 2018-03-22 7:25 ` Christoph Hellwig 2018-03-22 7:25 ` Christoph Hellwig 2018-03-30 15:16 ` Darrick J. Wong 2018-03-30 15:16 ` Darrick J. Wong 2018-03-21 22:58 ` [PATCH v7 13/14] xfs: prepare xfs_break_layouts() for another layout type Dan Williams 2018-03-21 22:58 ` Dan Williams 2018-03-22 7:27 ` Christoph Hellwig 2018-03-22 7:27 ` Christoph Hellwig 2018-03-22 15:50 ` Dan Williams 2018-03-22 15:50 ` Dan Williams 2018-03-21 22:58 ` [PATCH v7 14/14] xfs, dax: introduce xfs_break_dax_layouts() Dan Williams 2018-03-21 22:58 ` Dan Williams 2018-03-21 22:58 ` Dan Williams 2018-03-22 7:43 ` Christoph Hellwig 2018-03-22 7:43 ` Christoph Hellwig 2018-03-22 16:28 ` Dan Williams 2018-03-22 16:28 ` Dan Williams 2018-03-22 17:02 ` Christoph Hellwig 2018-03-22 17:02 ` Christoph Hellwig
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=20180329160215.glnrmyunujcc4vwg@quack2.suse.cz \ --to=jack@suse.cz \ --cc=dan.j.williams@intel.com \ --cc=david@fromorbit.com \ --cc=hch@lst.de \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-nvdimm@lists.01.org \ --cc=linux-xfs@vger.kernel.org \ --cc=mawilcox@microsoft.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: linkBe 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.