block: remove plugging at buffered write time

Buffered write(2) is not directly tied to IO, so it's not suitable to handle plug in generic_file_aio_write().

Buffered write(2) is not directly tied to IO, so it's not suitable to handle plug in generic_file_aio_write().

Also remove the redundant plug calls around ->direct_IO(), since we now do unplugging in the lower layer do_blockdev_direct_IO().

CC Jens Axboe <[email protected]>

CC Li Shaohua <[email protected]>

Signed-off-by Wu Fengguang <[email protected]>

---
 fs/direct-io.c |    4 ++++
 mm/filemap.c   |    7 -------
 2 files changed, 4 insertions(+), 7 deletions(-)

mm/filemap.c +0 -7

@@ -1421,12 +1421,8 @@ generic_file_aio_read(struct kiocb *iocb
 			retval = filemap_write_and_wait_range(mapping, pos,
 					pos + iov_length(iov, nr_segs) - 1);
 			if (!retval) {
-				struct blk_plug plug;
-
-				blk_start_plug(&plug);
 				retval = mapping->a_ops->direct_IO(READ, iocb,
 							iov, pos, nr_segs);
-				blk_finish_plug(&plug);
 			}
 			if (retval > 0) {
 				*ppos = pos + retval;

@@ -2610,13 +2606,11 @@ ssize_t generic_file_aio_write(struct ki
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *inode = file->f_mapping->host;
-	struct blk_plug plug;
 	ssize_t ret;
 
 	BUG_ON(iocb->ki_pos != pos);
 
 	mutex_lock(&inode->i_mutex);
-	blk_start_plug(&plug);
 	ret = __generic_file_aio_write(iocb, iov, nr_segs, &iocb->ki_pos);
 	mutex_unlock(&inode->i_mutex);
 

@@ -2627,7 +2621,6 @@ ssize_t generic_file_aio_write(struct ki
 		if (err < 0 && ret > 0)
 			ret = err;
 	}
-	blk_finish_plug(&plug);
 	return ret;
 }
 EXPORT_SYMBOL(generic_file_aio_write);

fs/direct-io.c +3 -0

@@ -1106,6 +1106,7 @@ do_blockdev_direct_IO(int rw, struct kio
 	unsigned long user_addr;
 	size_t bytes;
 	struct buffer_head map_bh = { 0, };
+	struct blk_plug plug;
 
 	if (rw & WRITE)
 		rw = WRITE_ODIRECT;

@@ -1221,6 +1222,8 @@ do_blockdev_direct_IO(int rw, struct kio
 				PAGE_SIZE - user_addr / PAGE_SIZE);
 	}
 
+	blk_start_plug(&plug);
+
 	for (seg = 0; seg < nr_segs; seg++) {
 		user_addr = (unsigned long)iov[seg].iov_base;
 		sdio.size += bytes = iov[seg].iov_len;

fs/direct-io.c +0 -0

 out:
 	return retval;
 }

Quoted 13 lines

(quoted diff) +2 -0

@@ -1221,6 +1222,8 @@ do_blockdev_direct_IO(int rw, struct kio
 				PAGE_SIZE - user_addr / PAGE_SIZE);
 	}
 
+	blk_start_plug(&plug);
+
 	for (seg = 0; seg < nr_segs; seg++) {
 		user_addr = (unsigned long)iov[seg].iov_base;
 		sdio.size += bytes = iov[seg].iov_len;

@@ -1314,6 +1317,7 @@ do_blockdev_direct_IO(int rw, struct kio
 	} else
 		BUG_ON(retval != -EIOCBQUEUED);
 

Review do_blockdev_direct_IO(int rw, struct kio · line 1320

Quoted 1 line

(quoted diff) +1 -0

+	blk_finish_plug(&plug);

This one can be moved up a little bit (before dio_cleanup)

Quoted 2 lines · Wu Fengguang

Buffered write(2) is not directly tied to IO, so it's not suitable to handle plug in generic_file_aio_write().

But generic_sync_write() does issue IO for O_SYNC writes, so unless there is plugging at a lower layer in the writeback code then it appears to me that plugging is still necessary (at least inside the sync branch)....

Done, thanks! Here is the updated patch with O_SYNC write fix.

Quoted 1 line

Done, thanks! Here is the updated patch with O_SYNC write fix.

Got these errors with the updated patch on top of 3.3-rc3:

fs/sync.c: In function 'vfs_fsync_range':
fs/sync.c:167:18: error: storage size of 'plug' isn't known
fs/sync.c:173:2: error: implicit declaration of function 'blk_start_plug' [-Werror=implicit-function-declaration] fs/sync.c:175:2: error: implicit declaration of function 'blk_finish_plug' [-Werror=implicit-function-declaration] fs/sync.c:167:18: warning: unused variable 'plug' [-Wunused-variable]

Quoted 9 lines

Quoted 1 line

Done, thanks! Here is the updated patch with O_SYNC write fix.

Got these errors with the updated patch on top of 3.3-rc3:

fs/sync.c: In function 'vfs_fsync_range':
fs/sync.c:167:18: error: storage size of 'plug' isn't known
fs/sync.c:173:2: error: implicit declaration of function 'blk_start_plug' [-Werror=implicit-function-declaration] fs/sync.c:175:2: error: implicit declaration of function 'blk_finish_plug' [-Werror=implicit-function-declaration] fs/sync.c:167:18: warning: unused variable 'plug' [-Wunused-variable]

Adding a #include <linux/blkdev.h> at the top of fs/sync.c solved the problem. Of course, this needs approval of a kernel developper before being added to the patch and resubmitted because I know adding #includes can have unwanted consequences sometimes or be seen as bloat...

Quoted 4 lines · Dave Chinner

Quoted 2 lines · Wu Fengguang

Buffered write(2) is not directly tied to IO, so it's not suitable to handle plug in generic_file_aio_write().

Quoted 4 lines

But generic_sync_write() does issue IO for O_SYNC writes, so unless there is plugging at a lower layer in the writeback code then it appears to me that plugging is still necessary (at least inside the sync branch)....

Good catch! It looks that generic_write_sync() eventually calls into vfs_fsync_range() which further calls ->fsync(). We may add plugging around it:

Quoted 10 lines · Wu Fengguang

Quoted 8 lines · Dave Chinner

Quoted 2 lines · Wu Fengguang

Buffered write(2) is not directly tied to IO, so it's not suitable to handle plug in generic_file_aio_write().

But generic_sync_write() does issue IO for O_SYNC writes, so unless there is plugging at a lower layer in the writeback code then it appears to me that plugging is still necessary (at least inside the sync branch)....

Quoted 3 lines

Good catch! It looks that generic_write_sync() eventually calls into vfs_fsync_range() which further calls ->fsync(). We may add plugging around it:

NAK, please keep the plugging down in the fs, or the libraries used but not common VFS code.

Quoted 16 lines · Christoph Hellwig

Quoted 13 lines · Wu Fengguang

Quoted 8 lines · Dave Chinner

Quoted 2 lines · Wu Fengguang

Buffered write(2) is not directly tied to IO, so it's not suitable to handle plug in generic_file_aio_write().

But generic_sync_write() does issue IO for O_SYNC writes, so unless there is plugging at a lower layer in the writeback code then it appears to me that plugging is still necessary (at least inside the sync branch)....

Good catch! It looks that generic_write_sync() eventually calls into vfs_fsync_range() which further calls ->fsync(). We may add plugging around it:

Quoted 2 lines

NAK, please keep the plugging down in the fs, or the libraries used but not common VFS code.

Please, what Christoph said. At least for btrfs plugging here is wrong.

Quoted 17 lines · Chris Mason

Quoted 16 lines · Christoph Hellwig

Quoted 13 lines · Wu Fengguang

Quoted 8 lines · Dave Chinner

Quoted 2 lines · Wu Fengguang

Buffered write(2) is not directly tied to IO, so it's not suitable to handle plug in generic_file_aio_write().

But generic_sync_write() does issue IO for O_SYNC writes, so unless there is plugging at a lower layer in the writeback code then it appears to me that plugging is still necessary (at least inside the sync branch)....

Good catch! It looks that generic_write_sync() eventually calls into vfs_fsync_range() which further calls ->fsync(). We may add plugging around it:

Quoted 4 lines

Quoted 2 lines

NAK, please keep the plugging down in the fs, or the libraries used but not common VFS code.

Please, what Christoph said. At least for btrfs plugging here is wrong.

OK, I get the point: the fs knows best when to unplug. Since any higher level plug nesting will turn such low level efforts into no-op, it's highly undesirable to do it in the high level.

Quoted 18 lines · Wu Fengguang

Quoted 17 lines · Chris Mason

Quoted 16 lines · Christoph Hellwig

Quoted 13 lines · Wu Fengguang

Quoted 8 lines · Dave Chinner

Quoted 2 lines · Wu Fengguang

Buffered write(2) is not directly tied to IO, so it's not suitable to handle plug in generic_file_aio_write().

But generic_sync_write() does issue IO for O_SYNC writes, so unless there is plugging at a lower layer in the writeback code then it appears to me that plugging is still necessary (at least inside the sync branch)....

Good catch! It looks that generic_write_sync() eventually calls into vfs_fsync_range() which further calls ->fsync(). We may add plugging around it:

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

NAK, please keep the plugging down in the fs, or the libraries used but not common VFS code.

Please, what Christoph said. At least for btrfs plugging here is wrong.

OK, I get the point: the fs knows best when to unplug. Since any higher level plug nesting will turn such low level efforts into no-op, it's highly undesirable to do it in the high level.

It's actually wrong to do plugging around vfs_fsync_range().

Because these call paths

        write() with O_SYNC
          generic_write_sync()
            vfs_fsync_range()
              ->fsync()
              generic_file_fsync()

        fsync()
          do_fsync()
            vfs_fsync()
              vfs_fsync_range()

pass arbitrary @size arguments, which may be much larger than the preferable I/O size, or may cross extent/device boundaries.

generic_file_fsync() starts with a filemap_write_and_wait_range() call, which already has proper plugging somewhere underneath. Then followed by metadata writes, which has plugging inside fsync_buffers_list(). At last, sync_inode_metadata() calls into ->write_inode() which may or may not care plugging.

The other fs specific ->fsync() do similar steps, varying in the metadata and fs specific housekeeping part.

I'll just drop this code. Shall the fs specific metadata I/O be plugged accordingly? I'm afraid this is beyond my knowledge base...