ocfs2: fix ip_unaligned_aio deadlock with dio work queue

Take ip_alloc_sem to prevent concurrent access to extent tree, which may cause the extent tree in an unstable state.

Signed-off-by Ryan Ding <[email protected]>

---
 fs/ocfs2/aops.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

fs/ocfs2/aops.c +17 -6

index cc604a2..d571509 100644
@@ -2151,6 +2151,7 @@ static int ocfs2_dio_get_block(struct inode *inode, sector_t iblock,
 			       struct buffer_head *bh_result, int create)
 {
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 	struct ocfs2_write_ctxt *wc;
 	struct ocfs2_write_cluster_desc *desc = NULL;
 	struct ocfs2_dio_write_ctxt *dwc = NULL;

@@ -2166,9 +2167,12 @@ static int ocfs2_dio_get_block(struct inode *inode, sector_t iblock,
 	mlog(0, "get block of %lu at %llu:%u req %u\n",
 			inode->i_ino, pos, len, total_len);
 
+	down_read(&oi->ip_alloc_sem);
 	/* This is the fast path for re-write. */
 	ret = ocfs2_get_block(inode, iblock, bh_result, create);
 
+	up_read(&oi->ip_alloc_sem);
+
 	if (buffer_mapped(bh_result) &&
 	    !buffer_new(bh_result) &&
 	    ret == 0)

@@ -2206,6 +2210,8 @@ static int ocfs2_dio_get_block(struct inode *inode, sector_t iblock,
 		goto out;
 	}
 
+	down_write(&oi->ip_alloc_sem);
+
 	if (first_get_block) {
 		if (ocfs2_sparse_alloc(OCFS2_SB(inode->i_sb)))
 			ret = ocfs2_zero_tail(inode, di_bh, pos);

@@ -2259,6 +2265,7 @@ static int ocfs2_dio_get_block(struct inode *inode, sector_t iblock,
 	BUG_ON(ret != len);
 	ret = 0;
 unlock:
+	up_write(&oi->ip_alloc_sem);
 	ocfs2_inode_unlock(inode, 1);
 	brelse(di_bh);
 out:

@@ -2275,6 +2282,7 @@ static void ocfs2_dio_end_io_write(struct inode *inode,
 	struct ocfs2_cached_dealloc_ctxt dealloc;
 	struct ocfs2_extent_tree et;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 	struct ocfs2_unwritten_extent *ue = NULL;
 	struct buffer_head *di_bh = NULL;
 	struct ocfs2_dinode *di;

@@ -2293,12 +2301,6 @@ static void ocfs2_dio_end_io_write(struct inode *inode,
 	    !dwc->dw_orphaned)
 		goto out;
 
-	ret = ocfs2_inode_lock(inode, &di_bh, 1);
-	if (ret < 0) {
-		mlog_errno(ret);
-		goto out;
-	}
-
 	/* ocfs2_file_write_iter will get i_mutex, so we need not lock if we
 	 * are in that context. */
 	if (dwc->dw_writer_pid != task_pid_nr(current)) {

@@ -2306,6 +2308,14 @@ static void ocfs2_dio_end_io_write(struct inode *inode,
 		locked = 1;
 	}
 
+	ret = ocfs2_inode_lock(inode, &di_bh, 1);
+	if (ret < 0) {
+		mlog_errno(ret);
+		goto out;
+	}
+
+	down_write(&oi->ip_alloc_sem);
+
 	/* Delete orphan before acquire i_mutex. */
 	if (dwc->dw_orphaned) {
 		BUG_ON(dwc->dw_writer_pid != task_pid_nr(current));

@@ -2359,6 +2369,7 @@ static void ocfs2_dio_end_io_write(struct inode *inode,
 commit:
 	ocfs2_commit_trans(osb, handle);
 unlock:
+	up_write(&oi->ip_alloc_sem);
 	ocfs2_inode_unlock(inode, 1);
 	brelse(di_bh);
 out:

When doing append direct write in an already allocated cluster, and fast path in ocfs2_dio_get_block() is triggered, function ocfs2_dio_end_io_write() will be skipped as there is no context allocated. So disk file size will not be changed as it should be.
The solution is to skip fast path when we are about to change file size.

Fixes af1310367f41 ("ocfs2: fix sparse file & data ordering issue in direct io.")

Signed-off-by Ryan Ding <[email protected]>

---
 fs/ocfs2/aops.c |   27 +++++++++++++++++----------
 1 files changed, 17 insertions(+), 10 deletions(-)

fs/ocfs2/aops.c +17 -10

index d571509..f33cfe4 100644
@@ -2167,19 +2167,26 @@ static int ocfs2_dio_get_block(struct inode *inode, sector_t iblock,
 	mlog(0, "get block of %lu at %llu:%u req %u\n",
 			inode->i_ino, pos, len, total_len);
 
-	down_read(&oi->ip_alloc_sem);
-	/* This is the fast path for re-write. */
-	ret = ocfs2_get_block(inode, iblock, bh_result, create);
+	/*
+	 * Because we need to change file size in ocfs2_dio_end_io_write(), or
+	 * we may need to add it to orphan dir. So can not fall to fast path
+	 * while file size will be changed.
+	 */
+	if (pos + total_len <= i_size_read(inode)) {
+		down_read(&oi->ip_alloc_sem);
+		/* This is the fast path for re-write. */
+		ret = ocfs2_get_block(inode, iblock, bh_result, create);
 
-	up_read(&oi->ip_alloc_sem);
+		up_read(&oi->ip_alloc_sem);
 
-	if (buffer_mapped(bh_result) &&
-	    !buffer_new(bh_result) &&
-	    ret == 0)
-		goto out;
+		if (buffer_mapped(bh_result) &&
+		    !buffer_new(bh_result) &&
+		    ret == 0)
+			goto out;
 
-	/* Clear state set by ocfs2_get_block. */
-	bh_result->b_state = 0;
+		/* Clear state set by ocfs2_get_block. */
+		bh_result->b_state = 0;
+	}
 
 	dwc = ocfs2_dio_alloc_write_ctx(bh_result, &first_get_block);
 	if (unlikely(dwc == NULL)) {

Should call ocfs2_free_alloc_context() to free meta_ac & data_ac before calling ocfs2_run_deallocs(). Because ocfs2_run_deallocs() will acquire the system inode's i_mutex hold by meta_ac. So try to release the lock before ocfs2_run_deallocs().

Fixes af1310367f41 ("ocfs2: fix sparse file & data ordering issue in direct io.")

Signed-off-by Ryan Ding <[email protected]>

---
 fs/ocfs2/aops.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

fs/ocfs2/aops.c +8 -4

index f33cfe4..182f754 100644
@@ -2341,6 +2341,10 @@ static void ocfs2_dio_end_io_write(struct inode *inode,
 
 	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
 				    &data_ac, &meta_ac);
+	if (ret) {
+		mlog_errno(ret);
+		goto unlock;
+	}
 
 	credits = ocfs2_calc_extend_credits(inode->i_sb, &di->id2.i_list);
 

@@ -2380,14 +2384,14 @@ unlock:
 	ocfs2_inode_unlock(inode, 1);
 	brelse(di_bh);
 out:
-	ocfs2_run_deallocs(osb, &dealloc);
-	if (locked)
-		mutex_unlock(&inode->i_mutex);
-	ocfs2_dio_free_write_ctx(inode, dwc);
 	if (data_ac)
 		ocfs2_free_alloc_context(data_ac);
 	if (meta_ac)
 		ocfs2_free_alloc_context(meta_ac);
+	ocfs2_run_deallocs(osb, &dealloc);
+	if (locked)
+		mutex_unlock(&inode->i_mutex);
+	ocfs2_dio_free_write_ctx(inode, dwc);
 }
 
 /*

In the current implementation of unaligned aio+dio, lock order behave as follow:

in user process context:
-> call io_submit()

    -> get i_mutex
		<== window1
      -> get ip_unaligned_aio
        -> submit direct io to block device
    -> release i_mutex

-> io_submit() return

in dio work queue context(the work queue is created in __blockdev_direct_IO):
-> release ip_unaligned_aio

		<== window2
    -> get i_mutex
      -> clear unwritten flag & change i_size
    -> release i_mutex

There is a limitation to the thread number of dio work queue. 256 at default. If all 256 thread are in the above 'window2' stage, and there is a user process in the 'window1' stage, the system will became deadlock. Since the user process hold i_mutex to wait ip_unaligned_aio lock, while there is a direct bio hold ip_unaligned_aio mutex who is waiting for a dio work queue thread to be schedule. But all the dio work queue thread is waiting for i_mutex lock in 'window2'.

This case only happened in a test which send a large number(more than 256) of aio at one io_submit() call.

My design is to remove ip_unaligned_aio lock. Change it to a sync io instead. Just like ip_unaligned_aio lock, serialize the unaligned aio dio.

Quoted 26 lines · On Fri, 20 Nov 2015 16:23:16 +0800 Ryan Ding <[email protected]> wrote:

In the current implementation of unaligned aio+dio, lock order behave as follow:

in user process context:

   -> call io_submit()
     -> get i_mutex
		<== window1
       -> get ip_unaligned_aio
         -> submit direct io to block device
     -> release i_mutex
   -> io_submit() return

in dio work queue context(the work queue is created in __blockdev_direct_IO):

   -> release ip_unaligned_aio
		<== window2
     -> get i_mutex
       -> clear unwritten flag & change i_size
     -> release i_mutex

There is a limitation to the thread number of dio work queue. 256 at default. If all 256 thread are in the above 'window2' stage, and there is a user process in the 'window1' stage, the system will became deadlock. Since the user process hold i_mutex to wait ip_unaligned_aio lock, while there is a direct bio hold ip_unaligned_aio mutex who is waiting for a dio work queue thread to be schedule. But all the dio work queue thread is waiting for i_mutex lock in 'window2'.

Quoted 5 lines

This case only happened in a test which send a large number(more than 256) of aio at one io_submit() call.

My design is to remove ip_unaligned_aio lock. Change it to a sync io instead. Just like ip_unaligned_aio lock, serialize the unaligned aio dio.

So this patch series is a bunch of fixes against your previous patch series:

ocfs2-add-ocfs2_write_type_t-type-to-identify-the-caller-of-write.patch ocfs2-use-c_new-to-indicate-newly-allocated-extents.patch ocfs2-test-target-page-before-change-it.patch ocfs2-do-not-change-i_size-in-write_end-for-direct-io.patch ocfs2-return-the-physical-address-in-ocfs2_write_cluster.patch ocfs2-record-unwritten-extents-when-populate-write-desc.patch ocfs2-fix-sparse-file-data-ordering-issue-in-direct-io.patch ocfs2-code-clean-up-for-direct-io.patch

correct?

Those patches are languishing a bit, awaiting review/ack. I'll send everything out for a round of review soon...

In the current implementation of unaligned aio+dio, lock order behave as follow:

in user process context:
-> call io_submit()

    -> get i_mutex
		<== window1
      -> get ip_unaligned_aio
        -> submit direct io to block device
    -> release i_mutex

-> io_submit() return

in dio work queue context(the work queue is created in __blockdev_direct_IO):
-> release ip_unaligned_aio

		<== window2
    -> get i_mutex
      -> clear unwritten flag & change i_size
    -> release i_mutex

There is a limitation to the thread number of dio work queue. 256 at default. If all 256 thread are in the above 'window2' stage, and there is a user process in the 'window1' stage, the system will became deadlock. Since the user process hold i_mutex to wait ip_unaligned_aio lock, while there is a direct bio hold ip_unaligned_aio mutex who is waiting for a dio work queue thread to be schedule. But all the dio work queue thread is waiting for i_mutex lock in 'window2'.

This case only happened in a test which send a large number(more than 256) of aio at one io_submit() call.

My design is to remove ip_unaligned_aio lock. Change it to a sync io instead. Just like ip_unaligned_aio lock, serialize the unaligned aio dio.

Signed-off-by Ryan Ding <[email protected]>

---
 fs/ocfs2/aops.c  |    6 ------
 fs/ocfs2/aops.h  |    7 -------
 fs/ocfs2/file.c  |   27 +++++++++------------------
 fs/ocfs2/inode.h |    3 ---
 fs/ocfs2/super.c |    1 -
 5 files changed, 9 insertions(+), 35 deletions(-)

fs/ocfs2/aops.c +0 -6

index 4bb9921..cc604a2 100644
@@ -2388,12 +2388,6 @@ static void ocfs2_dio_end_io(struct kiocb *iocb,
 	/* this io's submitter should not have unlocked this before we could */
 	BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
 
-	if (ocfs2_iocb_is_unaligned_aio(iocb)) {
-		ocfs2_iocb_clear_unaligned_aio(iocb);
-
-		mutex_unlock(&OCFS2_I(inode)->ip_unaligned_aio);
-	}
-
 	if (private)
 		ocfs2_dio_end_io_write(inode, private, offset, bytes);
 

fs/ocfs2/aops.h +0 -0

index d06b80f..7fc1189 100644

fs/ocfs2/aops.h +0 -0

 #endif /* OCFS2_FILE_H */

fs/ocfs2/file.c +9 -18

index 05346fb..6ab91cd 100644
@@ -2170,7 +2170,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	int full_coherency = !(osb->s_mount_opt &
 			       OCFS2_MOUNT_COHERENCY_BUFFERED);
-	int unaligned_dio = 0;
+	void *saved_ki_complete = NULL;
 	int append_write = ((iocb->ki_pos + count) >=
 			i_size_read(inode) ? 1 : 0);
 

@@ -2233,17 +2233,12 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 		goto out;
 	}
 
-	if (direct_io && !is_sync_kiocb(iocb))
-		unaligned_dio = ocfs2_is_io_unaligned(inode, count, iocb->ki_pos);
-
-	if (unaligned_dio) {
+	if (direct_io && !is_sync_kiocb(iocb) &&
+	    ocfs2_is_io_unaligned(inode, count, iocb->ki_pos)) {
 		/*
-		 * Wait on previous unaligned aio to complete before
-		 * proceeding.
+		 * Make it a sync io if it's an unaligned aio.
 		 */
-		mutex_lock(&OCFS2_I(inode)->ip_unaligned_aio);
-		/* Mark the iocb as needing an unlock in ocfs2_dio_end_io */
-		ocfs2_iocb_set_unaligned_aio(iocb);
+		saved_ki_complete = xchg(&iocb->ki_complete, NULL);
 	}
 
 	/* communicate with ocfs2_dio_end_io */

@@ -2264,11 +2259,10 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 	 */
 	if ((written == -EIOCBQUEUED) || (!ocfs2_iocb_is_rw_locked(iocb))) {
 		rw_level = -1;
-		unaligned_dio = 0;
 	}
 
 	if (unlikely(written <= 0))
-		goto no_sync;
+		goto out;
 
 	if (((file->f_flags & O_DSYNC) && !direct_io) ||
 	    IS_SYNC(inode)) {

@@ -2290,13 +2284,10 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 						      iocb->ki_pos - 1);
 	}
 
-no_sync:
-	if (unaligned_dio && ocfs2_iocb_is_unaligned_aio(iocb)) {
-		ocfs2_iocb_clear_unaligned_aio(iocb);
-		mutex_unlock(&OCFS2_I(inode)->ip_unaligned_aio);
-	}
-
 out:
+	if (saved_ki_complete)
+		xchg(&iocb->ki_complete, saved_ki_complete);
+
 	if (rw_level != -1)
 		ocfs2_rw_unlock(inode, rw_level);
 

fs/ocfs2/inode.h +0 -3

index 0c22ddd..6e0633e 100644
@@ -43,9 +43,6 @@ struct ocfs2_inode_info
 	/* protects extended attribute changes on this inode */
 	struct rw_semaphore		ip_xattr_sem;
 
-	/* Number of outstanding AIO's which are not page aligned */
-	struct mutex			ip_unaligned_aio;
-
 	/* These fields are protected by ip_lock */
 	spinlock_t			ip_lock;
 	u32				ip_open_count;

fs/ocfs2/super.c +0 -1

index c70a80b..3fd171f 100644
@@ -1723,7 +1723,6 @@ static void ocfs2_inode_init_once(void *data)
 	INIT_LIST_HEAD(&oi->ip_io_markers);
 	INIT_LIST_HEAD(&oi->ip_unwritten_list);
 	oi->ip_dir_start_lookup = 0;
-	mutex_init(&oi->ip_unaligned_aio);
 	init_rwsem(&oi->ip_alloc_sem);
 	init_rwsem(&oi->ip_xattr_sem);
 	mutex_init(&oi->ip_io_mutex);