ext4: introduce new i_write_mutex to protect fallocate

Introduce new i_write_mutex to protect new writes from coming while doing fallocate operations. Also, get rid of aio_mutex as it is covered by i_write_mutex.

Signed-off-by Namjae Jeon <[email protected]>

Signed-off-by Ashish Sangwan <[email protected]>

Quoted 4 lines · Namjae Jeon

Introduce new i_write_mutex to protect new writes from coming while doing fallocate operations. Also, get rid of aio_mutex as it is covered by i_write_mutex.

Thanks, applied.

Quoted 7 lines · Namjae Jeon

Quoted 4 lines

Introduce new i_write_mutex to protect new writes from coming while doing fallocate operations. Also, get rid of aio_mutex as it is covered by i_write_mutex.

Thanks, applied.

I had to apply the following fix up patch, or else running generic/308 using the ext3 config would end up returning to userspace with i_write_mutex still locked.

For all ext4 developers: please consider running at least "kvm-xfstests -g quick" (which would have shown this issue), or better yet, "kvm-xfstests -g auto" before sending patches. It finds problems like this. :-)

Cheers,

- Ted

fs/ext4/file.c +2 -3

index 8c39305..e5cd87f 100644
@@ -135,8 +135,8 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 		if ((pos > sbi->s_bitmap_maxbytes) ||
 		    (pos == sbi->s_bitmap_maxbytes && length > 0)) {
 			mutex_unlock(&inode->i_mutex);
-			ret = -EFBIG;
-			goto errout;
+			mutex_unlock(&EXT4_I(inode)->i_write_mutex);
+			return -EFBIG;
 		}
 
 		if (pos + length > sbi->s_bitmap_maxbytes) {

@@ -196,7 +196,6 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 	if (o_direct)
 		blk_finish_plug(&plug);
 
-errout:
 	if (unaligned_direct_aio)
 		mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 	return ret;

Date: Tue, 13 May 2014 09:19:17 +0900
From: Namjae Jeon <[email protected]>
To: Theodore Ts'o <[email protected]>
Cc: linux-ext4 <[email protected]>,
Lukáš Czerner <[email protected]>,
Ashish Sangwan <[email protected]>
Subject: [PATCH 1/2] ext4: introduce new i_write_mutex to protect fallocate

Introduce new i_write_mutex to protect new writes from coming while doing fallocate operations. Also, get rid of aio_mutex as it is covered by i_write_mutex.

Signed-off-by Namjae Jeon <[email protected]>

Signed-off-by Ashish Sangwan <[email protected]>

---
 fs/ext4/ext4.h    |  6 +++---
 fs/ext4/extents.c | 18 +++++++++++++++---
 fs/ext4/file.c    | 18 +++++++++++-------
 fs/ext4/inode.c   |  7 ++++++-
 fs/ext4/super.c   |  3 +--
 5 files changed, 36 insertions(+), 16 deletions(-)

fs/ext4/ext4.h +3 -3

index 6b45afa..77e5705 100644
@@ -943,6 +943,9 @@ struct ext4_inode_info {
 
 	/* Precomputed uuid+inum+igen checksum for seeding inode checksums */
 	__u32 i_csum_seed;
+
+	/* protects fallocate operations racing with new writes */
+	struct mutex i_write_mutex;
 };
 
 /*

@@ -2827,10 +2830,7 @@ static inline void ext4_inode_resume_unlocked_dio(struct inode *inode)
 #define EXT4_WQ_HASH_SZ		37
 #define ext4_ioend_wq(v)   (&ext4__ioend_wq[((unsigned long)(v)) %\
 					    EXT4_WQ_HASH_SZ])
-#define ext4_aio_mutex(v)  (&ext4__aio_mutex[((unsigned long)(v)) %\
-					     EXT4_WQ_HASH_SZ])
 extern wait_queue_head_t ext4__ioend_wq[EXT4_WQ_HASH_SZ];
-extern struct mutex ext4__aio_mutex[EXT4_WQ_HASH_SZ];
 
 #define EXT4_RESIZING	0
 extern int ext4_resize_begin(struct super_block *sb);

fs/ext4/extents.c +15 -3

index 086baa9..5262750 100644
@@ -4741,6 +4741,8 @@ static long ext4_zero_range(struct file *file, loff_t offset,
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
 
+	mutex_lock(&EXT4_I(inode)->i_write_mutex);
+
 	/*
 	 * Write out all dirty pages to avoid race conditions
 	 * Then release them.

@@ -4748,8 +4750,10 @@ static long ext4_zero_range(struct file *file, loff_t offset,
 	if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
 		ret = filemap_write_and_wait_range(mapping, offset,
 						   offset + len - 1);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 			return ret;
+		}
 	}
 
 	/*

@@ -4761,8 +4765,10 @@ static long ext4_zero_range(struct file *file, loff_t offset,
 	start = round_up(offset, 1 << blkbits);
 	end = round_down((offset + len), 1 << blkbits);
 
-	if (start < offset || end > offset + len)
+	if (start < offset || end > offset + len) {
+		mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 		return -EINVAL;
+	}
 	partial = (offset + len) & ((1 << blkbits) - 1);
 
 	lblk = start >> blkbits;

@@ -4859,6 +4865,7 @@ out_dio:
 	ext4_inode_resume_unlocked_dio(inode);
 out_mutex:
 	mutex_unlock(&inode->i_mutex);
+	mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 	return ret;
 }
 

@@ -5428,11 +5435,15 @@ int ext4_collapse_range(struct inode *inode, loff_t offset, loff_t len)
 	punch_start = offset >> EXT4_BLOCK_SIZE_BITS(sb);
 	punch_stop = (offset + len) >> EXT4_BLOCK_SIZE_BITS(sb);
 
+	mutex_lock(&EXT4_I(inode)->i_write_mutex);
+
 	/* Call ext4_force_commit to flush all data in case of data=journal. */
 	if (ext4_should_journal_data(inode)) {
 		ret = ext4_force_commit(inode->i_sb);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 			return ret;
+		}
 	}
 
 	/*

@@ -5518,5 +5529,6 @@ out_dio:
 	ext4_inode_resume_unlocked_dio(inode);
 out_mutex:
 	mutex_unlock(&inode->i_mutex);
+	mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 	return ret;
 }

fs/ext4/file.c +11 -7

index 708aad7..557b4ac 100644
@@ -93,7 +93,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(iocb->ki_filp);
-	struct mutex *aio_mutex = NULL;
+	bool unaligned_direct_aio = false;
 	struct blk_plug plug;
 	int o_direct = file->f_flags & O_DIRECT;
 	int overwrite = 0;

@@ -101,6 +101,8 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t ret;
 	loff_t pos = iocb->ki_pos;
 
+	mutex_lock(&EXT4_I(inode)->i_write_mutex);
+
 	/*
 	 * Unaligned direct AIO must be serialized; see comment above
 	 * In the case of O_APPEND, assume that we must always serialize

@@ -110,8 +112,7 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	    !is_sync_kiocb(iocb) &&
 	    (file->f_flags & O_APPEND ||
 	     ext4_unaligned_aio(inode, from, pos))) {
-		aio_mutex = ext4_aio_mutex(inode);
-		mutex_lock(aio_mutex);
+		unaligned_direct_aio = true;
 		ext4_unwritten_wait(inode);
 	}
 

@@ -143,8 +144,9 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		iocb->private = &overwrite;
 
 		/* check whether we do a DIO overwrite or not */
-		if (ext4_should_dioread_nolock(inode) && !aio_mutex &&
-		    !file->f_mapping->nrpages && pos + length <= i_size_read(inode)) {
+		if (ext4_should_dioread_nolock(inode) &&
+		    !unaligned_direct_aio && !file->f_mapping->nrpages &&
+		    pos + length <= i_size_read(inode)) {
 			struct ext4_map_blocks map;
 			unsigned int blkbits = inode->i_blkbits;
 			int err, len;

@@ -174,6 +176,8 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 
 	ret = __generic_file_write_iter(iocb, from);
 	mutex_unlock(&inode->i_mutex);
+	if (!unaligned_direct_aio)
+		mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 
 	if (ret > 0) {
 		ssize_t err;

@@ -186,8 +190,8 @@ ext4_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		blk_finish_plug(&plug);
 
 errout:
-	if (aio_mutex)
-		mutex_unlock(aio_mutex);
+	if (unaligned_direct_aio)
+		mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 	return ret;
 }
 

fs/ext4/inode.c +6 -1

index b1dc334..d804120 100644
@@ -3528,6 +3528,8 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 
 	trace_ext4_punch_hole(inode, offset, length, 0);
 
+	mutex_lock(&EXT4_I(inode)->i_write_mutex);
+
 	/*
 	 * Write out all dirty pages to avoid race conditions
 	 * Then release them.

@@ -3535,8 +3537,10 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
 	if (mapping->nrpages && mapping_tagged(mapping, PAGECACHE_TAG_DIRTY)) {
 		ret = filemap_write_and_wait_range(mapping, offset,
 						   offset + length - 1);
-		if (ret)
+		if (ret) {
+			mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 			return ret;
+		}
 	}
 
 	mutex_lock(&inode->i_mutex);

@@ -3637,6 +3641,7 @@ out_dio:
 	ext4_inode_resume_unlocked_dio(inode);
 out_mutex:
 	mutex_unlock(&inode->i_mutex);
+	mutex_unlock(&EXT4_I(inode)->i_write_mutex);
 	return ret;
 }
 

fs/ext4/super.c +1 -2

index 1f8cb18..e236c85 100644
@@ -904,6 +904,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
 	atomic_set(&ei->i_ioend_count, 0);
 	atomic_set(&ei->i_unwritten, 0);
 	INIT_WORK(&ei->i_rsv_conversion_work, ext4_end_io_rsv_work);
+	mutex_init(&ei->i_write_mutex);
 
 	return &ei->vfs_inode;
 }

@@ -5505,7 +5506,6 @@ static void ext4_exit_feat_adverts(void)
 
 /* Shared across all ext4 file systems */
 wait_queue_head_t ext4__ioend_wq[EXT4_WQ_HASH_SZ];
-struct mutex ext4__aio_mutex[EXT4_WQ_HASH_SZ];
 
 static int __init ext4_init_fs(void)
 {

@@ -5518,7 +5518,6 @@ static int __init ext4_init_fs(void)
 	ext4_check_flag_values();
 
 	for (i = 0; i < EXT4_WQ_HASH_SZ; i++) {
-		mutex_init(&ext4__aio_mutex[i]);
 		init_waitqueue_head(&ext4__ioend_wq[i]);
 	}
 

I wonder what is the performance impact of this change ? Especially since we're not longer taking the lock only in unaligned aio/dio case but in all cases ?

Quoted 4 lines

I wonder what is the performance impact of this change ? Especially since we're not longer taking the lock only in unaligned aio/dio case but in all cases ?

Thinking about this some more, this is also going to break parallel writes, which would be unfortunate. We might want to change this to using a rw mutex, where writes take a shared lock, and require fallocate to take an exclusive lock....

- Ted

ext4 file write is already serialized with inode mutex.

So I think the impact of adding another lock will be very very less..
When I run parallel write test of fio to prove it, I can not see the difference on w/wo i_write_mutex.

Yes, Right. We can use shared lock to remove a little bit lock contention in ext4 file write. I will share rwsem lock patch.. Could you please revert i_write_mutex patch ?

Quoted 1 line

ext4 file write is already serialized with inode mutex.

Right, I had forgotten about that. The case where we really care about parallel writes is in the direct I/O case, and eventually I'd like for us to be able to support non-overwriting/non-isize-extending writes in parallel but we're not there yet.

Quoted 3 lines

So I think the impact of adding another lock will be very very less.. When I run parallel write test of fio to prove it, I can not see the difference on w/wo

i_write_mutex.

If there is an impact, it won't show up there. Where it will show up will be in high scalability workloads. For people who don't have the half-million dollars (and up) expensive RAID arrays, a fairly good facsimile is to use a > 16 core system, preferably a system at least 4 sockets, and say 32 or 64 gigs of memory, of which you can dedicate half to a ramdisk. Then run the fio scalability benchmark in that scenario. That way, things like cache line bounces and lock contentions will be much more visible when the system is no longer bottleneck by the HDD.

Quoted 3 lines

Yes, Right. We can use shared lock to remove a little bit lock contention in ext4 file write. I will share rwsem lock patch.. Could you please revert i_write_mutex patch ?

So the shared lock will help somewhat (since writes will be far more common than fallocate calls) but I suspect, not all that much. And if I revert the i_write_mutex call, now, we won't have time to replace it with a different patch since the merge window is already open.

And since this patch is needed to fix a xfstests failure (although it's for collapse range in data journalling mode, so not a common case), if we can't really see a performance loss in the much more common server configurations, I'm inclined to leave it in for now, and we can try to improve performance in the next kernel revision.

What do other people think?

- Ted

Date: Tue, 03 Jun 2014 15:04:32 +0900
From: Namjae Jeon <[email protected]>
To: 'Theodore Ts'o' <[email protected]>
Cc: 'Lukáš Czerner' <[email protected]>,
'linux-ext4' <[email protected]>,
'Ashish Sangwan' <[email protected]>
Subject: RE: [PATCH 1/2] ext4: introduce new i_write_mutex to protect
fallocate

Quoted 8 lines · Namjae Jeon

Quoted 1 line

ext4 file write is already serialized with inode mutex.

Right, I had forgotten about that. The case where we really care about parallel writes is in the direct I/O case, and eventually I'd like for us to be able to support non-overwriting/non-isize-extending writes in parallel but we're not there yet.

Okay.

Quoted 5 lines

Quoted 3 lines

So I think the impact of adding another lock will be very very less.. When I run parallel write test of fio to prove it, I can not see the difference on w/wo

i_write_mutex.

Quoted 9 lines

If there is an impact, it won't show up there. Where it will show up will be in high scalability workloads. For people who don't have the half-million dollars (and up) expensive RAID arrays, a fairly good facsimile is to use a > 16 core system, preferably a system at least 4 sockets, and say 32 or 64 gigs of memory, of which you can dedicate half to a ramdisk. Then run the fio scalability benchmark in that scenario. That way, things like cache line bounces and lock contentions will be much more visible when the system is no longer bottleneck by the HDD.

Yes, Right. I agree that result should be measured on high-end server as you pointed again. Unfortunately I don't have such equipment yet..

Quoted 9 lines

Quoted 3 lines

Yes, Right. We can use shared lock to remove a little bit lock contention in ext4 file write. I will share rwsem lock patch.. Could you please revert i_write_mutex patch ?

So the shared lock will help somewhat (since writes will be far more common than fallocate calls) but I suspect, not all that much. And if I revert the i_write_mutex call, now, we won't have time to replace it with a different patch since the merge window is already open.

Quoted 5 lines

And since this patch is needed to fix a xfstests failure (although it's for collapse range in data journalling mode, so not a common case), if we can't really see a performance loss in the much more common server configurations, I'm inclined to leave it in for now, and we can try to improve performance in the next kernel revision.

IMHO, If our goal is to solve the problem of xfstests, we can use only "ext4: fix ZERO_RANGE test failure in data journalling" patch without i_write_mutex patch. And we can add lock for fallocate on next kernel after checking with sufficient time.

Thanks!

Quoted 4 lines

What do other people think?

- Ted

IMHO, If our goal is to solve the problem of xfstests, we can use only "ext4: fix ZERO_RANGE test failure in data journalling" patch without i_write_mutex patch. And we can add lock for fallocate on next kernel after checking with sufficient time.

Quoted 4 lines

IMHO, If our goal is to solve the problem of xfstests, we can use only "ext4: fix ZERO_RANGE test failure in data journalling" patch without i_write_mutex patch. And we can add lock for fallocate on next kernel after checking with sufficient time.

I thought this patch required i_write_mutex to avoid a race where another thread modifies an inode while filemap_write_and_wait_range() is running?

I agree that we could drop the i_write_mutex and add a call to ext4_force_commit() which should make the xfstest failure rarer, but the race would still be there, yes?

- Ted

Yes, It is there but as Lukas said it is not critical than a possible locking overhead. So, IMHO this is not something which needs urgent attention and can be tackled properly after checking unclear performance measurement on high-end server.