| Message ID | 1474992504-20133-19-git-send-email-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Sep 27, 2016 at 06:08:22PM +0200, Jan Kara wrote: > Currently, flushing of caches for DAX mappings was ignoring entry lock. > So far this was ok (modulo a bug that a difference in entry lock could > cause cache flushing to be mistakenly skipped) but in the following > patches we will write-protect PTEs on cache flushing and clear dirty > tags. For that we will need more exclusion. So do cache flushing under > an entry lock. This allows us to remove one lock-unlock pair of > mapping->tree_lock as a bonus. > > Signed-off-by: Jan Kara <jack@suse.cz> > @@ -716,15 +736,13 @@ static int dax_writeback_one(struct block_device *bdev, > } > > wb_cache_pmem(dax.addr, dax.size); > - > - spin_lock_irq(&mapping->tree_lock); > - radix_tree_tag_clear(page_tree, index, PAGECACHE_TAG_TOWRITE); > - spin_unlock_irq(&mapping->tree_lock); > - unmap: > +unmap: > dax_unmap_atomic(bdev, &dax); > + put_locked_mapping_entry(mapping, index, entry); > return ret; > > - unlock: > +put_unlock: I know there's an ongoing debate about this, but can you please stick a space in front of the labels to make the patches pretty & to be consistent with the rest of the DAX code? Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 18-10-16 13:20:13, Ross Zwisler wrote: > On Tue, Sep 27, 2016 at 06:08:22PM +0200, Jan Kara wrote: > > Currently, flushing of caches for DAX mappings was ignoring entry lock. > > So far this was ok (modulo a bug that a difference in entry lock could > > cause cache flushing to be mistakenly skipped) but in the following > > patches we will write-protect PTEs on cache flushing and clear dirty > > tags. For that we will need more exclusion. So do cache flushing under > > an entry lock. This allows us to remove one lock-unlock pair of > > mapping->tree_lock as a bonus. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > @@ -716,15 +736,13 @@ static int dax_writeback_one(struct block_device *bdev, > > } > > > > wb_cache_pmem(dax.addr, dax.size); > > - > > - spin_lock_irq(&mapping->tree_lock); > > - radix_tree_tag_clear(page_tree, index, PAGECACHE_TAG_TOWRITE); > > - spin_unlock_irq(&mapping->tree_lock); > > - unmap: > > +unmap: > > dax_unmap_atomic(bdev, &dax); > > + put_locked_mapping_entry(mapping, index, entry); > > return ret; > > > > - unlock: > > +put_unlock: > > I know there's an ongoing debate about this, but can you please stick a space > in front of the labels to make the patches pretty & to be consistent with the > rest of the DAX code? OK, done. > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> Thanks! Honza
On Tue, Oct 18, 2016 at 01:20:13PM -0600, Ross Zwisler wrote: > On Tue, Sep 27, 2016 at 06:08:22PM +0200, Jan Kara wrote: > > Currently, flushing of caches for DAX mappings was ignoring entry lock. > > So far this was ok (modulo a bug that a difference in entry lock could > > cause cache flushing to be mistakenly skipped) but in the following > > patches we will write-protect PTEs on cache flushing and clear dirty > > tags. For that we will need more exclusion. So do cache flushing under > > an entry lock. This allows us to remove one lock-unlock pair of > > mapping->tree_lock as a bonus. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > @@ -716,15 +736,13 @@ static int dax_writeback_one(struct block_device *bdev, > > } > > > > wb_cache_pmem(dax.addr, dax.size); > > - > > - spin_lock_irq(&mapping->tree_lock); > > - radix_tree_tag_clear(page_tree, index, PAGECACHE_TAG_TOWRITE); > > - spin_unlock_irq(&mapping->tree_lock); > > - unmap: > > +unmap: > > dax_unmap_atomic(bdev, &dax); > > + put_locked_mapping_entry(mapping, index, entry); > > return ret; > > > > - unlock: > > +put_unlock: > > I know there's an ongoing debate about this, but can you please stick a space > in front of the labels to make the patches pretty & to be consistent with the > rest of the DAX code? Never mind, it looks like the need for labels has been fixed with this commit: commit 218dd85887da (".gitattributes: set git diff driver for C source code files") With this, my git at least generates diffs that don't use labels in the chunk start, even without spaces. Feel free to leave out the spaces, and we can remove the stragglers from fs/dax.c at some point. Thanks, Jean, for fixing this! -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/dax.c b/fs/dax.c index b1c503930d1d..c6cadf8413a3 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -672,43 +672,63 @@ static int dax_writeback_one(struct block_device *bdev, struct address_space *mapping, pgoff_t index, void *entry) { struct radix_tree_root *page_tree = &mapping->page_tree; - int type = RADIX_DAX_TYPE(entry); - struct radix_tree_node *node; struct blk_dax_ctl dax; - void **slot; + void *entry2, **slot; int ret = 0; + int type; - spin_lock_irq(&mapping->tree_lock); /* - * Regular page slots are stabilized by the page lock even - * without the tree itself locked. These unlocked entries - * need verification under the tree lock. + * A page got tagged dirty in DAX mapping? Something is seriously + * wrong. */ - if (!__radix_tree_lookup(page_tree, index, &node, &slot)) - goto unlock; - if (*slot != entry) - goto unlock; - - /* another fsync thread may have already written back this entry */ - if (!radix_tree_tag_get(page_tree, index, PAGECACHE_TAG_TOWRITE)) - goto unlock; + if (WARN_ON(!radix_tree_exceptional_entry(entry))) + return -EIO; + spin_lock_irq(&mapping->tree_lock); + entry2 = get_unlocked_mapping_entry(mapping, index, &slot); + /* Entry got punched out / reallocated? */ + if (!entry2 || !radix_tree_exceptional_entry(entry2)) + goto put_unlock; + /* + * Entry got reallocated elsewhere? No need to writeback. We have to + * compare sectors as we must not bail out due to difference in lockbit + * or entry type. + */ + if (RADIX_DAX_SECTOR(entry2) != RADIX_DAX_SECTOR(entry)) + goto put_unlock; + type = RADIX_DAX_TYPE(entry2); if (WARN_ON_ONCE(type != RADIX_DAX_PTE && type != RADIX_DAX_PMD)) { ret = -EIO; - goto unlock; + goto put_unlock; } + /* Another fsync thread may have already written back this entry */ + if (!radix_tree_tag_get(page_tree, index, PAGECACHE_TAG_TOWRITE)) + goto put_unlock; + /* Lock the entry to serialize with page faults */ + entry = lock_slot(mapping, slot); + /* + * We can clear the tag now but we have to be careful so that concurrent + * dax_writeback_one() calls for the same index cannot finish before we + * actually flush the caches. This is achieved as the calls will look + * at the entry only under tree_lock and once they do that they will + * see the entry locked and wait for it to unlock. + */ + radix_tree_tag_clear(page_tree, index, PAGECACHE_TAG_TOWRITE); + spin_unlock_irq(&mapping->tree_lock); + dax.sector = RADIX_DAX_SECTOR(entry); dax.size = (type == RADIX_DAX_PMD ? PMD_SIZE : PAGE_SIZE); - spin_unlock_irq(&mapping->tree_lock); /* * We cannot hold tree_lock while calling dax_map_atomic() because it * eventually calls cond_resched(). */ ret = dax_map_atomic(bdev, &dax); - if (ret < 0) + if (ret < 0) { + put_locked_mapping_entry(mapping, index, entry); return ret; + } if (WARN_ON_ONCE(ret < dax.size)) { ret = -EIO; @@ -716,15 +736,13 @@ static int dax_writeback_one(struct block_device *bdev, } wb_cache_pmem(dax.addr, dax.size); - - spin_lock_irq(&mapping->tree_lock); - radix_tree_tag_clear(page_tree, index, PAGECACHE_TAG_TOWRITE); - spin_unlock_irq(&mapping->tree_lock); - unmap: +unmap: dax_unmap_atomic(bdev, &dax); + put_locked_mapping_entry(mapping, index, entry); return ret; - unlock: +put_unlock: + put_unlocked_mapping_entry(mapping, index, entry2); spin_unlock_irq(&mapping->tree_lock); return ret; }
Currently, flushing of caches for DAX mappings was ignoring entry lock. So far this was ok (modulo a bug that a difference in entry lock could cause cache flushing to be mistakenly skipped) but in the following patches we will write-protect PTEs on cache flushing and clear dirty tags. For that we will need more exclusion. So do cache flushing under an entry lock. This allows us to remove one lock-unlock pair of mapping->tree_lock as a bonus. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/dax.c | 66 +++++++++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 42 insertions(+), 24 deletions(-)