public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: xfs@oss.sgi.com
Subject: [PATCH 1/3] fs: get_blocks needs an unaligned mapping flag
Date: Fri, 23 Jul 2010 20:41:16 +1000	[thread overview]
Message-ID: <1279881678-1660-2-git-send-email-david@fromorbit.com> (raw)
In-Reply-To: <1279881678-1660-1-git-send-email-david@fromorbit.com>

From: Dave Chinner <dchinner@redhat.com>

When issuing concurrent unaligned direct IO to the same filesystem block, the
direct IO sub-block zeroing code will extend the length of the write being done
when writing into a hole or unwritten extents. If we are writing into unwritten
extents, then the two IOs will both see the extent as unwritten at IO issue
time and attempt to zero the part of the block that they are not writing to.

The result of this is that whichever IO completes last will win and part of the
block will be zero instead of containing the correct data. Eric Sandeen has
demonstrated the problem with xfstest #240. In the case of XFS, we allow
concurrent direct IO writes to occur, but we cannot allow block zeroing to
occur concurrently with other IO.

To allow serialisation of block zeroing across multiple independent IOs, we
need to know if the region being mapped by the IO is fsb-aligned or not. If it
is not aligned, then we need to prevent further direct IO writes from being
executed until the IO that is doing the zeroing completes (i.e. converts the
extent back to written). Passing the fact that the mapping is for an unaligned
IO into the get_blocks calback is sufficient to allow us to implement the
necessary serialisation.

Change the "create" parameter of the get_blocks callback to a flags field,
and define the flags to be backwards compatible as such:

#define GET_BLOCKS_READ		0x00	/* map, no allocation */
#define GET_BLOCKS_CREATE	0x01	/* map, allocate if hole */
#define GET_BLOCKS_UNALIGNED	0x02	/* mapping for unaligned IO */

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/buffer.c        |   17 +++++++++--------
 fs/direct-io.c     |   25 ++++++++++++++++++++++---
 fs/ioctl.c         |    2 +-
 fs/mpage.c         |    6 ++++--
 include/linux/fs.h |   10 +++++++++-
 5 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index d54812b..eda0395 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1680,7 +1680,7 @@ static int __block_write_full_page(struct inode *inode, struct page *page,
 		} else if ((!buffer_mapped(bh) || buffer_delay(bh)) &&
 			   buffer_dirty(bh)) {
 			WARN_ON(bh->b_size != blocksize);
-			err = get_block(inode, block, bh, 1);
+			err = get_block(inode, block, bh, GET_BLOCKS_CREATE);
 			if (err)
 				goto recover;
 			clear_buffer_delay(bh);
@@ -1869,7 +1869,7 @@ static int __block_prepare_write(struct inode *inode, struct page *page,
 			clear_buffer_new(bh);
 		if (!buffer_mapped(bh)) {
 			WARN_ON(bh->b_size != blocksize);
-			err = get_block(inode, block, bh, 1);
+			err = get_block(inode, block, bh, GET_BLOCKS_CREATE);
 			if (err)
 				break;
 			if (buffer_new(bh)) {
@@ -2192,7 +2192,8 @@ int block_read_full_page(struct page *page, get_block_t *get_block)
 			fully_mapped = 0;
 			if (iblock < lblock) {
 				WARN_ON(bh->b_size != blocksize);
-				err = get_block(inode, iblock, bh, 0);
+				err = get_block(inode, iblock, bh,
+							GET_BLOCKS_READ);
 				if (err)
 					SetPageError(page);
 			}
@@ -2583,9 +2584,9 @@ int nobh_write_begin_newtrunc(struct file *file, struct address_space *mapping,
 
 		block_end = block_start + blocksize;
 		bh->b_state = 0;
-		create = 1;
+		create = GET_BLOCKS_CREATE;
 		if (block_start >= to)
-			create = 0;
+			create = GET_BLOCKS_READ;
 		ret = get_block(inode, block_in_file + block_in_page,
 					bh, create);
 		if (ret)
@@ -2816,7 +2817,7 @@ has_buffers:
 
 	map_bh.b_size = blocksize;
 	map_bh.b_state = 0;
-	err = get_block(inode, iblock, &map_bh, 0);
+	err = get_block(inode, iblock, &map_bh, GET_BLOCKS_READ);
 	if (err)
 		goto unlock;
 	/* unmapped? It's a hole - nothing to do */
@@ -2893,7 +2894,7 @@ int block_truncate_page(struct address_space *mapping,
 	err = 0;
 	if (!buffer_mapped(bh)) {
 		WARN_ON(bh->b_size != blocksize);
-		err = get_block(inode, iblock, bh, 0);
+		err = get_block(inode, iblock, bh, GET_BLOCKS_READ);
 		if (err)
 			goto unlock;
 		/* unmapped? It's a hole - nothing to do */
@@ -2987,7 +2988,7 @@ sector_t generic_block_bmap(struct address_space *mapping, sector_t block,
 	tmp.b_state = 0;
 	tmp.b_blocknr = 0;
 	tmp.b_size = 1 << inode->i_blkbits;
-	get_block(inode, block, &tmp, 0);
+	get_block(inode, block, &tmp, GET_BLOCKS_READ);
 	return tmp.b_blocknr;
 }
 EXPORT_SYMBOL(generic_block_bmap);
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 7600aac..a120a3d 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -538,13 +538,33 @@ static int get_more_blocks(struct dio *dio)
 		dio_count = dio->final_block_in_request - dio->block_in_file;
 		fs_count = dio_count >> dio->blkfactor;
 		blkmask = (1 << dio->blkfactor) - 1;
-		if (dio_count & blkmask)	
+		if (dio_count & blkmask) {
 			fs_count++;
+		}
 
 		map_bh->b_state = 0;
 		map_bh->b_size = fs_count << dio->inode->i_blkbits;
 
 		/*
+		 * Because we're mapping full filesystem blocks here to do our
+		 * own sub-block zeroing for writes, we need to tell the fs
+		 * whether we might be doing zeroing so it can synchronise the
+		 * zeroing against other concurrent IO. If we don't do this,
+		 * the two concurrent sub-block IO's to the same fsb can race
+		 * and zero different portions of the sub-block resulting in
+		 * zeros where there should be data. Telling the fs we're doing
+		 * unaligned mappings allows it to serialise such IOs and avoid
+		 * the race condition.
+		 */
+		create = GET_BLOCKS_READ;
+		if (dio->rw & WRITE) {
+			create = GET_BLOCKS_CREATE;
+			if ((dio->block_in_file & blkmask) ||
+			    (dio->final_block_in_request & blkmask))
+				create |= GET_BLOCKS_UNALIGNED;
+		}
+
+		/*
 		 * For writes inside i_size on a DIO_SKIP_HOLES filesystem we
 		 * forbid block creations: only overwrites are permitted.
 		 * We will return early to the caller once we see an
@@ -555,11 +575,10 @@ static int get_more_blocks(struct dio *dio)
 		 * which may decide to handle it or also return an unmapped
 		 * buffer head.
 		 */
-		create = dio->rw & WRITE;
 		if (dio->flags & DIO_SKIP_HOLES) {
 			if (dio->block_in_file < (i_size_read(dio->inode) >>
 							dio->blkbits))
-				create = 0;
+				create = GET_BLOCKS_READ;
 		}
 
 		ret = (*dio->get_block)(dio->inode, fs_startblk,
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 2d140a7..3b6c095 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -295,7 +295,7 @@ int __generic_block_fiemap(struct inode *inode,
 		memset(&map_bh, 0, sizeof(struct buffer_head));
 		map_bh.b_size = len;
 
-		ret = get_block(inode, start_blk, &map_bh, 0);
+		ret = get_block(inode, start_blk, &map_bh, GET_BLOCKS_READ);
 		if (ret)
 			break;
 
diff --git a/fs/mpage.c b/fs/mpage.c
index fd56ca2..66d2acd 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -230,7 +230,8 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 
 		if (block_in_file < last_block) {
 			map_bh->b_size = (last_block-block_in_file) << blkbits;
-			if (get_block(inode, block_in_file, map_bh, 0))
+			if (get_block(inode, block_in_file, map_bh,
+							GET_BLOCKS_READ))
 				goto confused;
 			*first_logical_block = block_in_file;
 		}
@@ -533,7 +534,8 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc,
 
 		map_bh.b_state = 0;
 		map_bh.b_size = 1 << blkbits;
-		if (mpd->get_block(inode, block_in_file, &map_bh, 1))
+		if (mpd->get_block(inode, block_in_file, &map_bh,
+							GET_BLOCKS_CREATE))
 			goto confused;
 		if (buffer_new(&map_bh))
 			unmap_underlying_metadata(map_bh.b_bdev,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 68ca1b0..a140472 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -412,8 +412,16 @@ extern int dir_notify_enable;
 #endif
 
 struct buffer_head;
+
+/* get_blocks callback definition and flags */
+#define GET_BLOCKS_READ		0x00	/* map, no allocation */
+#define GET_BLOCKS_CREATE	0x01	/* map, allocate if hole */
+#define GET_BLOCKS_UNALIGNED	0x02	/* mapping for unaligned IO,
+					   only use with GET_BLOCKS_CREATE */
+
 typedef int (get_block_t)(struct inode *inode, sector_t iblock,
-			struct buffer_head *bh_result, int create);
+			struct buffer_head *bh_result, int flags);
+
 typedef void (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
 			ssize_t bytes, void *private);
 
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

  reply	other threads:[~2010-07-23 10:38 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-07-23 10:41 [RFC, PATCH 0/3] serialise concurrent direct IO sub-block zeroing Dave Chinner
2010-07-23 10:41 ` Dave Chinner [this message]
2010-07-23 15:29   ` [PATCH 1/3] fs: get_blocks needs an unaligned mapping flag Alex Elder
2010-07-23 10:41 ` [PATCH 2/3] xfs: serialise unaligned direct IO into unwritten extents Dave Chinner
2010-07-23 15:30   ` Alex Elder
2010-07-23 10:41 ` [PATCH 3/3] xfs: wait on IO completion inside an IO context Dave Chinner
2010-07-23 15:30   ` Alex Elder
2010-07-23 19:20 ` [RFC, PATCH 0/3] serialise concurrent direct IO sub-block zeroing Eric Sandeen
2010-07-24  0:09   ` Dave Chinner
2010-07-24 11:12     ` Alex Elder
2010-07-25 11:37       ` Dave Chinner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1279881678-1660-2-git-send-email-david@fromorbit.com \
    --to=david@fromorbit.com \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox