[RFC PATCH 0/5] Clean up ext4_file_write()

[RFC PATCH 0/5] Clean up ext4_file_write()

[Theodore Ts'o]

This patch series makes ext4_file_write() a bit easier to understand by
eliminating ext4_file_dio_write() and merging common code together.

In addition, fix an O_APPEND locking issue which Al Viro pointed out.

Theodore Ts'o (5):
  ext4: inline generic_file_aio_write() into ext4_file_write()
  ext4: move ext4_file_dio_write() into ext4_file_write()
  ext4: drop aio_mutex after grabbing i_mutex in ext4_file_write()
  ext4: factor out common code in ext4_file_write()
  ext4: add locking for O_APPEND writes

 fs/ext4/file.c | 153 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 78 insertions(+), 75 deletions(-)

-- 
1.9.0

[PATCH 1/5] ext4: inline generic_file_aio_write() into ext4_file_write()

[Theodore Ts'o]

Copy generic_file_aio_write() into ext4_file_write().  This is part of
a patch series which allows us to simplify ext4_file_write() and
ext4_file_dio_write(), by calling __generic_file_aio_write() directly.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/file.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 25a20b6..8b22556 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -113,8 +113,6 @@ ext4_file_dio_write(struct kiocb *iocb, const struct iovec *iov,
 		ext4_unwritten_wait(inode);
 	}
 
-	BUG_ON(iocb->ki_pos != pos);
-
 	mutex_lock(&inode->i_mutex);
 	blk_start_plug(&plug);
 
@@ -168,9 +166,12 @@ static ssize_t
 ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 		unsigned long nr_segs, loff_t pos)
 {
+	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(iocb->ki_filp);
 	ssize_t ret;
 
+	BUG_ON(iocb->ki_pos != pos);
+
 	/*
 	 * If we have encountered a bitmap-format file, the size limit
 	 * is smaller than s_maxbytes, which is for extent-mapped files.
@@ -192,8 +193,20 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 
 	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT))
 		ret = ext4_file_dio_write(iocb, iov, nr_segs, pos);
-	else
-		ret = generic_file_aio_write(iocb, iov, nr_segs, pos);
+	else {
+		mutex_lock(&inode->i_mutex);
+		ret = __generic_file_aio_write(iocb, iov, nr_segs,
+					       &iocb->ki_pos);
+		mutex_unlock(&inode->i_mutex);
+
+		if (ret > 0) {
+			ssize_t err;
+
+			err = generic_write_sync(file, iocb->ki_pos - ret, ret);
+			if (err < 0)
+				ret = err;
+		}
+	}
 
 	return ret;
 }
-- 
1.9.0

Re: [PATCH 1/5] ext4: inline generic_file_aio_write() into ext4_file_write()

[Jan Kara]

On Sat 12-04-14 13:02:37, Ted Tso wrote:
> Copy generic_file_aio_write() into ext4_file_write().  This is part of
> a patch series which allows us to simplify ext4_file_write() and
> ext4_file_dio_write(), by calling __generic_file_aio_write() directly.
  Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/file.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 25a20b6..8b22556 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -113,8 +113,6 @@ ext4_file_dio_write(struct kiocb *iocb, const struct iovec *iov,
>  		ext4_unwritten_wait(inode);
>  	}
>  
> -	BUG_ON(iocb->ki_pos != pos);
> -
>  	mutex_lock(&inode->i_mutex);
>  	blk_start_plug(&plug);
>  
> @@ -168,9 +166,12 @@ static ssize_t
>  ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  		unsigned long nr_segs, loff_t pos)
>  {
> +	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  	ssize_t ret;
>  
> +	BUG_ON(iocb->ki_pos != pos);
> +
>  	/*
>  	 * If we have encountered a bitmap-format file, the size limit
>  	 * is smaller than s_maxbytes, which is for extent-mapped files.
> @@ -192,8 +193,20 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  
>  	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT))
>  		ret = ext4_file_dio_write(iocb, iov, nr_segs, pos);
> -	else
> -		ret = generic_file_aio_write(iocb, iov, nr_segs, pos);
> +	else {
> +		mutex_lock(&inode->i_mutex);
> +		ret = __generic_file_aio_write(iocb, iov, nr_segs,
> +					       &iocb->ki_pos);
> +		mutex_unlock(&inode->i_mutex);
> +
> +		if (ret > 0) {
> +			ssize_t err;
> +
> +			err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> +			if (err < 0)
> +				ret = err;
> +		}
> +	}
>  
>  	return ret;
>  }
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 2/5] ext4: move ext4_file_dio_write() into ext4_file_write()

[Theodore Ts'o]

This commit doesn't actually change anything; it just moves code
around in preparation for some code simplification work.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/file.c | 139 +++++++++++++++++++++++++++------------------------------
 1 file changed, 65 insertions(+), 74 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 8b22556..3ec0c09 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -92,82 +92,15 @@ ext4_unaligned_aio(struct inode *inode, const struct iovec *iov,
 }
 
 static ssize_t
-ext4_file_dio_write(struct kiocb *iocb, const struct iovec *iov,
-		    unsigned long nr_segs, loff_t pos)
+ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
+		unsigned long nr_segs, loff_t pos)
 {
 	struct file *file = iocb->ki_filp;
-	struct inode *inode = file->f_mapping->host;
+	struct inode *inode = file_inode(iocb->ki_filp);
 	struct blk_plug plug;
 	int unaligned_aio = 0;
-	ssize_t ret;
 	int overwrite = 0;
 	size_t length = iov_length(iov, nr_segs);
-
-	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
-	    !is_sync_kiocb(iocb))
-		unaligned_aio = ext4_unaligned_aio(inode, iov, nr_segs, pos);
-
-	/* Unaligned direct AIO must be serialized; see comment above */
-	if (unaligned_aio) {
-		mutex_lock(ext4_aio_mutex(inode));
-		ext4_unwritten_wait(inode);
-	}
-
-	mutex_lock(&inode->i_mutex);
-	blk_start_plug(&plug);
-
-	iocb->private = &overwrite;
-
-	/* check whether we do a DIO overwrite or not */
-	if (ext4_should_dioread_nolock(inode) && !unaligned_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;
-
-		map.m_lblk = pos >> blkbits;
-		map.m_len = (EXT4_BLOCK_ALIGN(pos + length, blkbits) >> blkbits)
-			- map.m_lblk;
-		len = map.m_len;
-
-		err = ext4_map_blocks(NULL, inode, &map, 0);
-		/*
-		 * 'err==len' means that all of blocks has been preallocated no
-		 * matter they are initialized or not.  For excluding
-		 * unwritten extents, we need to check m_flags.  There are
-		 * two conditions that indicate for initialized extents.
-		 * 1) If we hit extent cache, EXT4_MAP_MAPPED flag is returned;
-		 * 2) If we do a real lookup, non-flags are returned.
-		 * So we should check these two conditions.
-		 */
-		if (err == len && (map.m_flags & EXT4_MAP_MAPPED))
-			overwrite = 1;
-	}
-
-	ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);
-	mutex_unlock(&inode->i_mutex);
-
-	if (ret > 0) {
-		ssize_t err;
-
-		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-		if (err < 0)
-			ret = err;
-	}
-	blk_finish_plug(&plug);
-
-	if (unaligned_aio)
-		mutex_unlock(ext4_aio_mutex(inode));
-
-	return ret;
-}
-
-static ssize_t
-ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
-		unsigned long nr_segs, loff_t pos)
-{
-	struct file *file = iocb->ki_filp;
-	struct inode *inode = file_inode(iocb->ki_filp);
 	ssize_t ret;
 
 	BUG_ON(iocb->ki_pos != pos);
@@ -179,7 +112,6 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 
 	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
 		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
-		size_t length = iov_length(iov, nr_segs);
 
 		if ((pos > sbi->s_bitmap_maxbytes ||
 		    (pos == sbi->s_bitmap_maxbytes && length > 0)))
@@ -191,9 +123,68 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 		}
 	}
 
-	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT))
-		ret = ext4_file_dio_write(iocb, iov, nr_segs, pos);
-	else {
+	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT)) {
+		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
+		    !is_sync_kiocb(iocb))
+			unaligned_aio = ext4_unaligned_aio(inode, iov,
+							   nr_segs, pos);
+
+		/* Unaligned direct AIO must be serialized; see comment above */
+		if (unaligned_aio) {
+			mutex_lock(ext4_aio_mutex(inode));
+			ext4_unwritten_wait(inode);
+		}
+
+		mutex_lock(&inode->i_mutex);
+		blk_start_plug(&plug);
+
+		iocb->private = &overwrite;
+
+		/* check whether we do a DIO overwrite or not */
+		if (ext4_should_dioread_nolock(inode) && !unaligned_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;
+
+			map.m_lblk = pos >> blkbits;
+			map.m_len = (EXT4_BLOCK_ALIGN(pos + length, blkbits) >> blkbits)
+				- map.m_lblk;
+			len = map.m_len;
+
+			err = ext4_map_blocks(NULL, inode, &map, 0);
+			/*
+			 * 'err==len' means that all of blocks has
+			 * been preallocated no matter they are
+			 * initialized or not.  For excluding
+			 * unwritten extents, we need to check
+			 * m_flags.  There are two conditions that
+			 * indicate for initialized extents.  1) If we
+			 * hit extent cache, EXT4_MAP_MAPPED flag is
+			 * returned; 2) If we do a real lookup,
+			 * non-flags are returned.  So we should check
+			 * these two conditions.
+			 */
+			if (err == len && (map.m_flags & EXT4_MAP_MAPPED))
+				overwrite = 1;
+		}
+
+		ret = __generic_file_aio_write(iocb, iov, nr_segs,
+					       &iocb->ki_pos);
+		mutex_unlock(&inode->i_mutex);
+
+		if (ret > 0) {
+			ssize_t err;
+
+			err = generic_write_sync(file, iocb->ki_pos - ret, ret);
+			if (err < 0)
+				ret = err;
+		}
+		blk_finish_plug(&plug);
+
+		if (unaligned_aio)
+			mutex_unlock(ext4_aio_mutex(inode));
+	} else {
 		mutex_lock(&inode->i_mutex);
 		ret = __generic_file_aio_write(iocb, iov, nr_segs,
 					       &iocb->ki_pos);
-- 
1.9.0

Re: [PATCH 2/5] ext4: move ext4_file_dio_write() into ext4_file_write()

[Jan Kara]

On Sat 12-04-14 13:02:38, Ted Tso wrote:
> This commit doesn't actually change anything; it just moves code
> around in preparation for some code simplification work.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
  Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/file.c | 139 +++++++++++++++++++++++++++------------------------------
>  1 file changed, 65 insertions(+), 74 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 8b22556..3ec0c09 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -92,82 +92,15 @@ ext4_unaligned_aio(struct inode *inode, const struct iovec *iov,
>  }
>  
>  static ssize_t
> -ext4_file_dio_write(struct kiocb *iocb, const struct iovec *iov,
> -		    unsigned long nr_segs, loff_t pos)
> +ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
> +		unsigned long nr_segs, loff_t pos)
>  {
>  	struct file *file = iocb->ki_filp;
> -	struct inode *inode = file->f_mapping->host;
> +	struct inode *inode = file_inode(iocb->ki_filp);
>  	struct blk_plug plug;
>  	int unaligned_aio = 0;
> -	ssize_t ret;
>  	int overwrite = 0;
>  	size_t length = iov_length(iov, nr_segs);
> -
> -	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
> -	    !is_sync_kiocb(iocb))
> -		unaligned_aio = ext4_unaligned_aio(inode, iov, nr_segs, pos);
> -
> -	/* Unaligned direct AIO must be serialized; see comment above */
> -	if (unaligned_aio) {
> -		mutex_lock(ext4_aio_mutex(inode));
> -		ext4_unwritten_wait(inode);
> -	}
> -
> -	mutex_lock(&inode->i_mutex);
> -	blk_start_plug(&plug);
> -
> -	iocb->private = &overwrite;
> -
> -	/* check whether we do a DIO overwrite or not */
> -	if (ext4_should_dioread_nolock(inode) && !unaligned_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;
> -
> -		map.m_lblk = pos >> blkbits;
> -		map.m_len = (EXT4_BLOCK_ALIGN(pos + length, blkbits) >> blkbits)
> -			- map.m_lblk;
> -		len = map.m_len;
> -
> -		err = ext4_map_blocks(NULL, inode, &map, 0);
> -		/*
> -		 * 'err==len' means that all of blocks has been preallocated no
> -		 * matter they are initialized or not.  For excluding
> -		 * unwritten extents, we need to check m_flags.  There are
> -		 * two conditions that indicate for initialized extents.
> -		 * 1) If we hit extent cache, EXT4_MAP_MAPPED flag is returned;
> -		 * 2) If we do a real lookup, non-flags are returned.
> -		 * So we should check these two conditions.
> -		 */
> -		if (err == len && (map.m_flags & EXT4_MAP_MAPPED))
> -			overwrite = 1;
> -	}
> -
> -	ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);
> -	mutex_unlock(&inode->i_mutex);
> -
> -	if (ret > 0) {
> -		ssize_t err;
> -
> -		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> -		if (err < 0)
> -			ret = err;
> -	}
> -	blk_finish_plug(&plug);
> -
> -	if (unaligned_aio)
> -		mutex_unlock(ext4_aio_mutex(inode));
> -
> -	return ret;
> -}
> -
> -static ssize_t
> -ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
> -		unsigned long nr_segs, loff_t pos)
> -{
> -	struct file *file = iocb->ki_filp;
> -	struct inode *inode = file_inode(iocb->ki_filp);
>  	ssize_t ret;
>  
>  	BUG_ON(iocb->ki_pos != pos);
> @@ -179,7 +112,6 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  
>  	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
>  		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> -		size_t length = iov_length(iov, nr_segs);
>  
>  		if ((pos > sbi->s_bitmap_maxbytes ||
>  		    (pos == sbi->s_bitmap_maxbytes && length > 0)))
> @@ -191,9 +123,68 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  		}
>  	}
>  
> -	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT))
> -		ret = ext4_file_dio_write(iocb, iov, nr_segs, pos);
> -	else {
> +	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT)) {
> +		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
> +		    !is_sync_kiocb(iocb))
> +			unaligned_aio = ext4_unaligned_aio(inode, iov,
> +							   nr_segs, pos);
> +
> +		/* Unaligned direct AIO must be serialized; see comment above */
> +		if (unaligned_aio) {
> +			mutex_lock(ext4_aio_mutex(inode));
> +			ext4_unwritten_wait(inode);
> +		}
> +
> +		mutex_lock(&inode->i_mutex);
> +		blk_start_plug(&plug);
> +
> +		iocb->private = &overwrite;
> +
> +		/* check whether we do a DIO overwrite or not */
> +		if (ext4_should_dioread_nolock(inode) && !unaligned_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;
> +
> +			map.m_lblk = pos >> blkbits;
> +			map.m_len = (EXT4_BLOCK_ALIGN(pos + length, blkbits) >> blkbits)
> +				- map.m_lblk;
> +			len = map.m_len;
> +
> +			err = ext4_map_blocks(NULL, inode, &map, 0);
> +			/*
> +			 * 'err==len' means that all of blocks has
> +			 * been preallocated no matter they are
> +			 * initialized or not.  For excluding
> +			 * unwritten extents, we need to check
> +			 * m_flags.  There are two conditions that
> +			 * indicate for initialized extents.  1) If we
> +			 * hit extent cache, EXT4_MAP_MAPPED flag is
> +			 * returned; 2) If we do a real lookup,
> +			 * non-flags are returned.  So we should check
> +			 * these two conditions.
> +			 */
> +			if (err == len && (map.m_flags & EXT4_MAP_MAPPED))
> +				overwrite = 1;
> +		}
> +
> +		ret = __generic_file_aio_write(iocb, iov, nr_segs,
> +					       &iocb->ki_pos);
> +		mutex_unlock(&inode->i_mutex);
> +
> +		if (ret > 0) {
> +			ssize_t err;
> +
> +			err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> +			if (err < 0)
> +				ret = err;
> +		}
> +		blk_finish_plug(&plug);
> +
> +		if (unaligned_aio)
> +			mutex_unlock(ext4_aio_mutex(inode));
> +	} else {
>  		mutex_lock(&inode->i_mutex);
>  		ret = __generic_file_aio_write(iocb, iov, nr_segs,
>  					       &iocb->ki_pos);
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 3/5] ext4: drop aio_mutex after grabbing i_mutex in ext4_file_write()

[Theodore Ts'o]

Once we have grabbed i_mutex, this will prevent any other writes from
proceeding, so we don't need to worry about any writes to unwritten
regions from taking place.  So drop the aio_mutex so that if there are
any other parallel writes happen to some other inode that happens to
hash to the same hash table entry, we won't end up blocking that work.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/file.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 3ec0c09..5e428d58 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -98,7 +98,6 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(iocb->ki_filp);
 	struct blk_plug plug;
-	int unaligned_aio = 0;
 	int overwrite = 0;
 	size_t length = iov_length(iov, nr_segs);
 	ssize_t ret;
@@ -124,24 +123,26 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 	}
 
 	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT)) {
-		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
-		    !is_sync_kiocb(iocb))
-			unaligned_aio = ext4_unaligned_aio(inode, iov,
-							   nr_segs, pos);
+		struct mutex *aio_mutex = NULL;
 
 		/* Unaligned direct AIO must be serialized; see comment above */
-		if (unaligned_aio) {
-			mutex_lock(ext4_aio_mutex(inode));
+		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
+		    !is_sync_kiocb(iocb) &&
+		    ext4_unaligned_aio(inode, iov, nr_segs, pos)) {
+			aio_mutex = ext4_aio_mutex(inode);
+			mutex_lock(aio_mutex);
 			ext4_unwritten_wait(inode);
 		}
 
 		mutex_lock(&inode->i_mutex);
+		if (aio_mutex)
+			mutex_unlock(aio_mutex);
 		blk_start_plug(&plug);
 
 		iocb->private = &overwrite;
 
 		/* check whether we do a DIO overwrite or not */
-		if (ext4_should_dioread_nolock(inode) && !unaligned_aio &&
+		if (ext4_should_dioread_nolock(inode) && !aio_mutex &&
 		    !file->f_mapping->nrpages && pos + length <= i_size_read(inode)) {
 			struct ext4_map_blocks map;
 			unsigned int blkbits = inode->i_blkbits;
@@ -181,9 +182,6 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 				ret = err;
 		}
 		blk_finish_plug(&plug);

Re: [PATCH 3/5] ext4: drop aio_mutex after grabbing i_mutex in ext4_file_write()

[Jan Kara]

On Sat 12-04-14 13:02:39, Ted Tso wrote:
> Once we have grabbed i_mutex, this will prevent any other writes from
> proceeding, so we don't need to worry about any writes to unwritten
> regions from taking place.  So drop the aio_mutex so that if there are
> any other parallel writes happen to some other inode that happens to
> hash to the same hash table entry, we won't end up blocking that work.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
  Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> ---
>  fs/ext4/file.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 3ec0c09..5e428d58 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -98,7 +98,6 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  	struct blk_plug plug;
> -	int unaligned_aio = 0;
>  	int overwrite = 0;
>  	size_t length = iov_length(iov, nr_segs);
>  	ssize_t ret;
> @@ -124,24 +123,26 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  	}
>  
>  	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT)) {
> -		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
> -		    !is_sync_kiocb(iocb))
> -			unaligned_aio = ext4_unaligned_aio(inode, iov,
> -							   nr_segs, pos);
> +		struct mutex *aio_mutex = NULL;
>  
>  		/* Unaligned direct AIO must be serialized; see comment above */
> -		if (unaligned_aio) {
> -			mutex_lock(ext4_aio_mutex(inode));
> +		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
> +		    !is_sync_kiocb(iocb) &&
> +		    ext4_unaligned_aio(inode, iov, nr_segs, pos)) {
> +			aio_mutex = ext4_aio_mutex(inode);
> +			mutex_lock(aio_mutex);
>  			ext4_unwritten_wait(inode);
>  		}
>  
>  		mutex_lock(&inode->i_mutex);
> +		if (aio_mutex)
> +			mutex_unlock(aio_mutex);
>  		blk_start_plug(&plug);
>  
>  		iocb->private = &overwrite;
>  
>  		/* check whether we do a DIO overwrite or not */
> -		if (ext4_should_dioread_nolock(inode) && !unaligned_aio &&
> +		if (ext4_should_dioread_nolock(inode) && !aio_mutex &&
>  		    !file->f_mapping->nrpages && pos + length <= i_size_read(inode)) {
>  			struct ext4_map_blocks map;
>  			unsigned int blkbits = inode->i_blkbits;
> @@ -181,9 +182,6 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  				ret = err;
>  		}
>  		blk_finish_plug(&plug);
> -
> -		if (unaligned_aio)
> -			mutex_unlock(ext4_aio_mutex(inode));
>  	} else {
>  		mutex_lock(&inode->i_mutex);
>  		ret = __generic_file_aio_write(iocb, iov, nr_segs,
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 4/5] ext4: factor out common code in ext4_file_write()

[Theodore Ts'o]

This shouldn't change any logic flow; just delete duplicated code.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/file.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 5e428d58..7c8f483 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -98,6 +98,7 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(iocb->ki_filp);
 	struct blk_plug plug;
+	int o_direct = file->f_flags & O_DIRECT;
 	int overwrite = 0;
 	size_t length = iov_length(iov, nr_segs);
 	ssize_t ret;
@@ -122,7 +123,7 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 		}
 	}
 
-	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT)) {
+	if (o_direct) {
 		struct mutex *aio_mutex = NULL;
 
 		/* Unaligned direct AIO must be serialized; see comment above */
@@ -169,33 +170,21 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 			if (err == len && (map.m_flags & EXT4_MAP_MAPPED))
 				overwrite = 1;
 		}
-
-		ret = __generic_file_aio_write(iocb, iov, nr_segs,
-					       &iocb->ki_pos);
-		mutex_unlock(&inode->i_mutex);
-
-		if (ret > 0) {
-			ssize_t err;
-
-			err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-			if (err < 0)
-				ret = err;
-		}
-		blk_finish_plug(&plug);
-	} else {
+	} else
 		mutex_lock(&inode->i_mutex);
-		ret = __generic_file_aio_write(iocb, iov, nr_segs,
-					       &iocb->ki_pos);
-		mutex_unlock(&inode->i_mutex);
 
-		if (ret > 0) {
-			ssize_t err;
+	ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);
+	mutex_unlock(&inode->i_mutex);
 
-			err = generic_write_sync(file, iocb->ki_pos - ret, ret);
-			if (err < 0)
-				ret = err;
-		}
+	if (ret > 0) {
+		ssize_t err;
+
+		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
+		if (err < 0)
+			ret = err;
 	}
+	if (o_direct)
+		blk_finish_plug(&plug);
 
 	return ret;
 }
-- 
1.9.0

Re: [PATCH 4/5] ext4: factor out common code in ext4_file_write()

[Jan Kara]

On Sat 12-04-14 13:02:40, Ted Tso wrote:
> This shouldn't change any logic flow; just delete duplicated code.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
  Looks good. You can add:
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/file.c | 37 +++++++++++++------------------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 5e428d58..7c8f483 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -98,6 +98,7 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  	struct blk_plug plug;
> +	int o_direct = file->f_flags & O_DIRECT;
>  	int overwrite = 0;
>  	size_t length = iov_length(iov, nr_segs);
>  	ssize_t ret;
> @@ -122,7 +123,7 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  		}
>  	}
>  
> -	if (unlikely(iocb->ki_filp->f_flags & O_DIRECT)) {
> +	if (o_direct) {
>  		struct mutex *aio_mutex = NULL;
>  
>  		/* Unaligned direct AIO must be serialized; see comment above */
> @@ -169,33 +170,21 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  			if (err == len && (map.m_flags & EXT4_MAP_MAPPED))
>  				overwrite = 1;
>  		}
> -
> -		ret = __generic_file_aio_write(iocb, iov, nr_segs,
> -					       &iocb->ki_pos);
> -		mutex_unlock(&inode->i_mutex);
> -
> -		if (ret > 0) {
> -			ssize_t err;
> -
> -			err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> -			if (err < 0)
> -				ret = err;
> -		}
> -		blk_finish_plug(&plug);
> -	} else {
> +	} else
>  		mutex_lock(&inode->i_mutex);
> -		ret = __generic_file_aio_write(iocb, iov, nr_segs,
> -					       &iocb->ki_pos);
> -		mutex_unlock(&inode->i_mutex);
>  
> -		if (ret > 0) {
> -			ssize_t err;
> +	ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);
> +	mutex_unlock(&inode->i_mutex);
>  
> -			err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> -			if (err < 0)
> -				ret = err;
> -		}
> +	if (ret > 0) {
> +		ssize_t err;
> +
> +		err = generic_write_sync(file, iocb->ki_pos - ret, ret);
> +		if (err < 0)
> +			ret = err;
>  	}
> +	if (o_direct)
> +		blk_finish_plug(&plug);
>  
>  	return ret;
>  }
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 5/5] ext4: add locking for O_APPEND writes

[Theodore Ts'o]

Al Viro pointed out that we need to make sure we only allow one
O_APPEND write to proceed at a time so that the the s_bitmap_maxbytes
check can be properly checked.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/file.c | 46 +++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 7c8f483..c3824dd 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -98,24 +98,32 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file_inode(iocb->ki_filp);
 	struct blk_plug plug;
+	struct mutex *aio_mutex = NULL;
 	int o_direct = file->f_flags & O_DIRECT;
-	int overwrite = 0;
+	int overwrite = 0, i_mutex_grabbed = 0;
 	size_t length = iov_length(iov, nr_segs);
 	ssize_t ret;
 
 	BUG_ON(iocb->ki_pos != pos);
 
+	if (file->f_flags & O_APPEND) {
+		mutex_lock(&inode->i_mutex);
+		i_mutex_grabbed = 1;
+		iocb->ki_pos = pos = i_size_read(inode);
+	}
+
 	/*
 	 * If we have encountered a bitmap-format file, the size limit
 	 * is smaller than s_maxbytes, which is for extent-mapped files.
 	 */
-
 	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
 		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 
 		if ((pos > sbi->s_bitmap_maxbytes ||
-		    (pos == sbi->s_bitmap_maxbytes && length > 0)))
-			return -EFBIG;
+		     (pos == sbi->s_bitmap_maxbytes && length > 0))) {
+			ret = -EFBIG;
+			goto errout;
+		}
 
 		if (pos + length > sbi->s_bitmap_maxbytes) {
 			nr_segs = iov_shorten((struct iovec *)iov, nr_segs,
@@ -123,19 +131,20 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 		}
 	}
 
-	if (o_direct) {
-		struct mutex *aio_mutex = NULL;
-
-		/* Unaligned direct AIO must be serialized; see comment above */
-		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
-		    !is_sync_kiocb(iocb) &&
-		    ext4_unaligned_aio(inode, iov, nr_segs, pos)) {
-			aio_mutex = ext4_aio_mutex(inode);
-			mutex_lock(aio_mutex);
-			ext4_unwritten_wait(inode);
-		}
+	/* Unaligned direct AIO must be serialized; see comment above */
+	if (o_direct && ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
+	    !is_sync_kiocb(iocb) &&
+	    ext4_unaligned_aio(inode, iov, nr_segs, pos)) {
+		aio_mutex = ext4_aio_mutex(inode);
+		mutex_lock(aio_mutex);
+		ext4_unwritten_wait(inode);
+	}
 
+	if (!i_mutex_grabbed)
 		mutex_lock(&inode->i_mutex);
+	i_mutex_grabbed = 1;
+
+	if (o_direct) {
 		if (aio_mutex)
 			mutex_unlock(aio_mutex);
 		blk_start_plug(&plug);
@@ -170,11 +179,11 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 			if (err == len && (map.m_flags & EXT4_MAP_MAPPED))
 				overwrite = 1;
 		}
-	} else
-		mutex_lock(&inode->i_mutex);
+	}
 
 	ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);
 	mutex_unlock(&inode->i_mutex);
+	i_mutex_grabbed = 0;
 
 	if (ret > 0) {
 		ssize_t err;
@@ -186,6 +195,9 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 	if (o_direct)
 		blk_finish_plug(&plug);
 
+errout:
+	if (i_mutex_grabbed)
+		mutex_unlock(&inode->i_mutex);
 	return ret;
 }
 
-- 
1.9.0

Re: [PATCH 5/5] ext4: add locking for O_APPEND writes

[Jan Kara]

On Sat 12-04-14 13:02:41, Ted Tso wrote:
> Al Viro pointed out that we need to make sure we only allow one
> O_APPEND write to proceed at a time so that the the s_bitmap_maxbytes
> check can be properly checked.
  But this introduces lock inversion between aio_mutex and i_mutex, doesn't
it?

								Honza

> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/file.c | 46 +++++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 7c8f483..c3824dd 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -98,24 +98,32 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  	struct file *file = iocb->ki_filp;
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  	struct blk_plug plug;
> +	struct mutex *aio_mutex = NULL;
>  	int o_direct = file->f_flags & O_DIRECT;
> -	int overwrite = 0;
> +	int overwrite = 0, i_mutex_grabbed = 0;
>  	size_t length = iov_length(iov, nr_segs);
>  	ssize_t ret;
>  
>  	BUG_ON(iocb->ki_pos != pos);
>  
> +	if (file->f_flags & O_APPEND) {
> +		mutex_lock(&inode->i_mutex);
> +		i_mutex_grabbed = 1;
> +		iocb->ki_pos = pos = i_size_read(inode);
> +	}
> +
>  	/*
>  	 * If we have encountered a bitmap-format file, the size limit
>  	 * is smaller than s_maxbytes, which is for extent-mapped files.
>  	 */
> -
>  	if (!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))) {
>  		struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>  
>  		if ((pos > sbi->s_bitmap_maxbytes ||
> -		    (pos == sbi->s_bitmap_maxbytes && length > 0)))
> -			return -EFBIG;
> +		     (pos == sbi->s_bitmap_maxbytes && length > 0))) {
> +			ret = -EFBIG;
> +			goto errout;
> +		}
>  
>  		if (pos + length > sbi->s_bitmap_maxbytes) {
>  			nr_segs = iov_shorten((struct iovec *)iov, nr_segs,
> @@ -123,19 +131,20 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  		}
>  	}
>  
> -	if (o_direct) {
> -		struct mutex *aio_mutex = NULL;
> -
> -		/* Unaligned direct AIO must be serialized; see comment above */
> -		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
> -		    !is_sync_kiocb(iocb) &&
> -		    ext4_unaligned_aio(inode, iov, nr_segs, pos)) {
> -			aio_mutex = ext4_aio_mutex(inode);
> -			mutex_lock(aio_mutex);
> -			ext4_unwritten_wait(inode);
> -		}
> +	/* Unaligned direct AIO must be serialized; see comment above */
> +	if (o_direct && ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS) &&
> +	    !is_sync_kiocb(iocb) &&
> +	    ext4_unaligned_aio(inode, iov, nr_segs, pos)) {
> +		aio_mutex = ext4_aio_mutex(inode);
> +		mutex_lock(aio_mutex);
> +		ext4_unwritten_wait(inode);
> +	}
>  
> +	if (!i_mutex_grabbed)
>  		mutex_lock(&inode->i_mutex);
> +	i_mutex_grabbed = 1;
> +
> +	if (o_direct) {
>  		if (aio_mutex)
>  			mutex_unlock(aio_mutex);
>  		blk_start_plug(&plug);
> @@ -170,11 +179,11 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  			if (err == len && (map.m_flags & EXT4_MAP_MAPPED))
>  				overwrite = 1;
>  		}
> -	} else
> -		mutex_lock(&inode->i_mutex);
> +	}
>  
>  	ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);
>  	mutex_unlock(&inode->i_mutex);
> +	i_mutex_grabbed = 0;
>  
>  	if (ret > 0) {
>  		ssize_t err;
> @@ -186,6 +195,9 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  	if (o_direct)
>  		blk_finish_plug(&plug);
>  
> +errout:
> +	if (i_mutex_grabbed)
> +		mutex_unlock(&inode->i_mutex);
>  	return ret;
>  }
>  
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 5/5] ext4: add locking for O_APPEND writes

[Theodore Ts'o]

On Tue, Apr 15, 2014 at 07:05:40PM +0200, Jan Kara wrote:
> On Sat 12-04-14 13:02:41, Ted Tso wrote:
> > Al Viro pointed out that we need to make sure we only allow one
> > O_APPEND write to proceed at a time so that the the s_bitmap_maxbytes
> > check can be properly checked.
>   But this introduces lock inversion between aio_mutex and i_mutex, doesn't
> it?

Doh!  Thanks for pointing that out.  Fortunately, we don't need to
care about optimizing the AIO/DIO O_APPEND write case, so probably the
best thing to do is to unconditionally take aio_mutex and call
ext4_unwritten_wait() early, before we grab i_mutex.  So in effect
we'll treat all O_APPEND writes as being unaligned in and in need of
serialization.

I'll send out a revised version for this last patch.

     	      			    	      - Ted