[Cosmic] [PATCH 0/3] Fixes for LP:1787089

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[Cosmic] [PATCH 0/3] Fixes for LP:1787089

Joseph Salisbury-3
BugLink: https://bugs.launchpad.net/bugs/1787089

== Cosmic Justification ==
Intel is requsting these three commits in Cosmic.  

Even with the ext4_break_layouts() support added by "ext4: handle layout
changes to pinned DAX mappings" Still seeing occasional cases with unit
 test where we are truncating a page that has an elevated reference count.


== Fixes ==
cdbf8897cb09 ("dax: dax_layout_busy_page() warn on !exceptional")
430657b6be89 ("ext4: handle layout changes to pinned DAX mappings")
e25ff835af89 ("xfs: Close race between direct IO and xfs_break_layouts()")

Dave Jiang (1):
  xfs: Close race between direct IO and xfs_break_layouts()

Ross Zwisler (2):
  dax: dax_layout_busy_page() warn on !exceptional
  ext4: handle layout changes to pinned DAX mappings

 fs/dax.c           | 10 +++++++++-
 fs/ext4/ext4.h     |  1 +
 fs/ext4/extents.c  | 17 +++++++++++++++++
 fs/ext4/inode.c    | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/truncate.h |  4 ++++
 fs/xfs/xfs_file.c  |  9 ++++-----
 6 files changed, 81 insertions(+), 6 deletions(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Cosmic][PATCH 1/3] dax: dax_layout_busy_page() warn on !exceptional

Joseph Salisbury-3
From: Ross Zwisler <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1787089

Inodes using DAX should only ever have exceptional entries in their page
caches.  Make this clear by warning if the iteration in
dax_layout_busy_page() ever sees a non-exceptional entry, and by adding a
comment for the pagevec_release() call which only deals with struct page
pointers.

Signed-off-by: Ross Zwisler <[hidden email]>
Signed-off-by: Theodore Ts'o <[hidden email]>
Reviewed-by: Jan Kara <[hidden email]>
(cherry picked from commit cdbf8897cb09b7baf2b8a7e78051a35a872b01d5)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 fs/dax.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/dax.c b/fs/dax.c
index 57ec272..48d3eb9 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -673,7 +673,8 @@ struct page *dax_layout_busy_page(struct address_space *mapping)
  if (index >= end)
  break;
 
- if (!radix_tree_exceptional_entry(pvec_ent))
+ if (WARN_ON_ONCE(
+     !radix_tree_exceptional_entry(pvec_ent)))
  continue;
 
  xa_lock_irq(&mapping->i_pages);
@@ -685,6 +686,13 @@ struct page *dax_layout_busy_page(struct address_space *mapping)
  if (page)
  break;
  }
+
+ /*
+ * We don't expect normal struct page entries to exist in our
+ * tree, but we keep these pagevec calls so that this code is
+ * consistent with the common pattern for handling pagevecs
+ * throughout the kernel.
+ */
  pagevec_remove_exceptionals(&pvec);
  pagevec_release(&pvec);
  index++;
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Cosmic][PATCH 2/3] ext4: handle layout changes to pinned DAX mappings

Joseph Salisbury-3
In reply to this post by Joseph Salisbury-3
From: Ross Zwisler <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1787089

Follow the lead of xfs_break_dax_layouts() and add synchronization between
operations in ext4 which remove blocks from an inode (hole punch, truncate
down, etc.) and pages which are pinned due to DAX DMA operations.

Signed-off-by: Ross Zwisler <[hidden email]>
Signed-off-by: Theodore Ts'o <[hidden email]>
Reviewed-by: Jan Kara <[hidden email]>
Reviewed-by: Lukas Czerner <[hidden email]>
(cherry picked from commit 430657b6be896db57d974375cc499ca212c7f01d)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 fs/ext4/ext4.h     |  1 +
 fs/ext4/extents.c  | 17 +++++++++++++++++
 fs/ext4/inode.c    | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/ext4/truncate.h |  4 ++++
 4 files changed, 68 insertions(+)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 7c7123f..f84b401 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2456,6 +2456,7 @@ extern int ext4_get_inode_loc(struct inode *, struct ext4_iloc *);
 extern int ext4_inode_attach_jinode(struct inode *inode);
 extern int ext4_can_truncate(struct inode *inode);
 extern int ext4_truncate(struct inode *);
+extern int ext4_break_layouts(struct inode *);
 extern int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length);
 extern int ext4_truncate_restart_trans(handle_t *, struct inode *, int nblocks);
 extern void ext4_set_inode_flags(struct inode *);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 8ce6fd5..72a361d 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4826,6 +4826,13 @@ static long ext4_zero_range(struct file *file, loff_t offset,
  * released from page cache.
  */
  down_write(&EXT4_I(inode)->i_mmap_sem);
+
+ ret = ext4_break_layouts(inode);
+ if (ret) {
+ up_write(&EXT4_I(inode)->i_mmap_sem);
+ goto out_mutex;
+ }
+
  ret = ext4_update_disksize_before_punch(inode, offset, len);
  if (ret) {
  up_write(&EXT4_I(inode)->i_mmap_sem);
@@ -5499,6 +5506,11 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
  * page cache.
  */
  down_write(&EXT4_I(inode)->i_mmap_sem);
+
+ ret = ext4_break_layouts(inode);
+ if (ret)
+ goto out_mmap;
+
  /*
  * Need to round down offset to be aligned with page size boundary
  * for page size > block size.
@@ -5647,6 +5659,11 @@ int ext4_insert_range(struct inode *inode, loff_t offset, loff_t len)
  * page cache.
  */
  down_write(&EXT4_I(inode)->i_mmap_sem);
+
+ ret = ext4_break_layouts(inode);
+ if (ret)
+ goto out_mmap;
+
  /*
  * Need to round down to align start offset to page size boundary
  * for page size > block size.
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index df27a70..9adc59c 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4191,6 +4191,39 @@ int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset,
  return 0;
 }
 
+static void ext4_wait_dax_page(struct ext4_inode_info *ei, bool *did_unlock)
+{
+ *did_unlock = true;
+ up_write(&ei->i_mmap_sem);
+ schedule();
+ down_write(&ei->i_mmap_sem);
+}
+
+int ext4_break_layouts(struct inode *inode)
+{
+ struct ext4_inode_info *ei = EXT4_I(inode);
+ struct page *page;
+ bool retry;
+ int error;
+
+ if (WARN_ON_ONCE(!rwsem_is_locked(&ei->i_mmap_sem)))
+ return -EINVAL;
+
+ do {
+ retry = false;
+ page = dax_layout_busy_page(inode->i_mapping);
+ if (!page)
+ return 0;
+
+ error = ___wait_var_event(&page->_refcount,
+ atomic_read(&page->_refcount) == 1,
+ TASK_INTERRUPTIBLE, 0, 0,
+ ext4_wait_dax_page(ei, &retry));
+ } while (error == 0 && retry);
+
+ return error;
+}
+
 /*
  * ext4_punch_hole: punches a hole in a file by releasing the blocks
  * associated with the given offset and length
@@ -4264,6 +4297,11 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
  * page cache.
  */
  down_write(&EXT4_I(inode)->i_mmap_sem);
+
+ ret = ext4_break_layouts(inode);
+ if (ret)
+ goto out_dio;
+
  first_block_offset = round_up(offset, sb->s_blocksize);
  last_block_offset = round_down((offset + length), sb->s_blocksize) - 1;
 
@@ -5555,6 +5593,14 @@ int ext4_setattr(struct dentry *dentry, struct iattr *attr)
  ext4_wait_for_tail_page_commit(inode);
  }
  down_write(&EXT4_I(inode)->i_mmap_sem);
+
+ rc = ext4_break_layouts(inode);
+ if (rc) {
+ up_write(&EXT4_I(inode)->i_mmap_sem);
+ error = rc;
+ goto err_out;
+ }
+
  /*
  * Truncate pagecache after we've waited for commit
  * in data=journal mode to make pages freeable.
diff --git a/fs/ext4/truncate.h b/fs/ext4/truncate.h
index 0cb13ba..bcbe366 100644
--- a/fs/ext4/truncate.h
+++ b/fs/ext4/truncate.h
@@ -11,6 +11,10 @@
  */
 static inline void ext4_truncate_failed_write(struct inode *inode)
 {
+ /*
+ * We don't need to call ext4_break_layouts() because the blocks we
+ * are truncating were never visible to userspace.
+ */
  down_write(&EXT4_I(inode)->i_mmap_sem);
  truncate_inode_pages(inode->i_mapping, inode->i_size);
  ext4_truncate(inode);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[Cosmic][PATCH 3/3] xfs: Close race between direct IO and xfs_break_layouts()

Joseph Salisbury-3
In reply to this post by Joseph Salisbury-3
From: Dave Jiang <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1787089

This patch is the duplicate of ross's fix for ext4 for xfs.

If the refcount of a page is lowered between the time that it is returned
by dax_busy_page() and when the refcount is again checked in
xfs_break_layouts() => ___wait_var_event(), the waiting function
xfs_wait_dax_page() will never be called.  This means that
xfs_break_layouts() will still have 'retry' set to false, so we'll stop
looping and never check the refcount of other pages in this inode.

Instead, always continue looping as long as dax_layout_busy_page() gives us
a page which it found with an elevated refcount.

Signed-off-by: Dave Jiang <[hidden email]>
Reviewed-by: Jan Kara <[hidden email]>
Reviewed-by: Darrick J. Wong <[hidden email]>
Signed-off-by: Darrick J. Wong <[hidden email]>
(backported from commit e25ff835af89a80aa6a4de58f413e494b2b96bd1)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 fs/xfs/xfs_file.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index a3e7767..cd6f0d8 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -721,12 +721,10 @@ xfs_file_write_iter(
 
 static void
 xfs_wait_dax_page(
- struct inode *inode,
- bool *did_unlock)
+ struct inode *inode)
 {
  struct xfs_inode        *ip = XFS_I(inode);
 
- *did_unlock = true;
  xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
  schedule();
  xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
@@ -736,7 +734,7 @@ static int
 xfs_break_dax_layouts(
  struct inode *inode,
  uint iolock,
- bool *did_unlock)
+ bool *retry)
 {
  struct page *page;
 
@@ -746,9 +744,10 @@ xfs_break_dax_layouts(
  if (!page)
  return 0;
 
+ *retry = true;
  return ___wait_var_event(&page->_refcount,
  atomic_read(&page->_refcount) == 1, TASK_INTERRUPTIBLE,
- 0, 0, xfs_wait_dax_page(inode, did_unlock));
+ 0, 0, xfs_wait_dax_page(inode));
 }
 
 int
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [Cosmic] [PATCH 0/3] Fixes for LP:1787089

Thadeu Lima de Souza Cascardo-3
In reply to this post by Joseph Salisbury-3
Applied to cosmic master-next branch.

Thanks.
Cascardo.

Applied-to: cosmic/master-next

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team