All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
@ 2023-03-07 20:49 Boris Burkov
  2023-03-07 23:47 ` kernel test robot
                   ` (6 more replies)
  0 siblings, 7 replies; 10+ messages in thread
From: Boris Burkov @ 2023-03-07 20:49 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

If an application is doing direct io to a btrfs file and experiences a
page fault reading from the write buffer, iomap will issue a partial
bio, and allow the fs to keep going. However, there was a subtle bug in
this code path in the btrfs dio iomap implementation that led to the
partial write ending up as a gap in the file's extents and to be read
back as zeros.

The sequence of events in a partial write, lightly summarized and
trimmed down for brevity is as follows:

  ====WRITING TASK====
  btrfs_direct_write
  __iomap_dio_write
  iomap_iter
    btrfs_dio_iomap_begin # create full ordered extent
  iomap_dio_bio_iter
    bio_iov_iter_get_pages # page fault; partial read
    submit_bio # partial bio
  iomap_iter
    btrfs_dio_iomap_end
      btrfs_mark_ordered_io_finished # sets BTRFS_ORDERED_IOERR;
				     # submit to finish_ordered_fn wq
  fault_in_iov_iter_readable # btrfs_direct_write detects partial write
  __iomap_dio_write
  iomap_iter
    btrfs_dio_iomap_begin # create second partial ordered extent
  iomap_dio_bio_iter
    bio_iov_iter_get_pages # read all of remainder
    submit_bio # partial bio with all of remainder
  iomap_iter
    btrfs_dio_iomap_end # nothing exciting to do with ordered io

  ====DIO ENDIO====
  ==FIRST PARTIAL BIO==
  btrfs_dio_end_io
    btrfs_mark_ordered_io_finished # bytes_left > 0
				   # don't submit to finish_ordered_fn wq
  ==SECOND PARTIAL BIO==
  btrfs_dio_end_io
    btrfs_mark_ordered_io_finished # bytes_left == 0
				   # submit to finish_ordered_fn wq

  ====BTRFS FINISH ORDERED WQ====
  ==FIRST PARTIAL BIO==
  btrfs_finish_ordered_io # called by dio_iomap_end_io, sees
			  # BTRFS_ORDERED_IOERR, just drops the
			  # ordered_extent
  ==SECOND PARTIAL BIO==
  btrfs_finish_ordered_io # called by btrfs_dio_end_io, writes out file
			  # extents, csums, etc...

The essence of the problem is that while btrfs_direct_write and iomap
properly interact to submit all the correct bios, there is insufficient
logic in the btrfs dio functions (btrfs_dio_iomap_begin,
btrfs_dio_submit_io, btrfs_dio_end_io, and btrfs_dio_iomap_end) to
ensure that every bio is at least a part of a completed ordered_extent.
And it is completing an ordered_extent that results in crucial
functionality like inserting a file extent item for the range in the
subvolume/fs tree.

More specifically, btrfs_dio_end_io treats the ordered extent as
unfinished but btrfs_dio_iomap_end sets BTRFS_ORDERED_IOERR on it.
Thus, the finish io work doesn't result in file extents, csums, etc...
In the aftermath, such a file behaves as though it has a hole in it,
instead of the purportedly written data.

We considered a few options for fixing the bug:

1. treat the partial bio as if we had truncated the file, which would
   result in properly finishing it.
2. split the ordered extent when submitting a partial bio.
3. cache the ordered extent across calls to __iomap_dio_rw in
   iter->private, so that we could reuse it and correctly apply several
   bios to it.

I had trouble with 1, and it felt the most like a hack, so I tried 2
and 3. Since 3 has the benefit of also not creating an extra file
extent, and avoids an ordered extent lookup during bio submission, it
felt like the best option.

A quick summary of the changes necessary to implement this cached
ordered_extent behavior:

- btrfs_direct_write keeps track of an ordered_extent for the duration
  of a call, possible across several __iomap_dio_rws.
- zero the btrfs_dio_data before using it, since its fields constitute
  state now.
- btrfs_dio_write uses dio_data to pass this ordered extent into and out
  of __iomap_dio_rw.
- when the write is done, put the ordered_extent.
- if the short write happens to be length 0, then we _don't_ get an
  extra bio, so we do need to cancel the ordered_extent like we used
  to (and ditch the cached ordered extent)
- if the short write ordered_extent has an error on it, drop the cached
  ordered extent, as before.
- in btrfs_dio_iomap_begin, if the cached ordered extent is present,
  skip all the work of creating it, just look up the extent mapping and
  jump to setting up the iomap. (This part could likely be more
  elegant..)

Thanks to Josef, Christoph, and Filipe with their help figuring out the
bug and the fix.

Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes")
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2169947
Link: https://lore.kernel.org/linux-btrfs/aa1fb69e-b613-47aa-a99e-a0a2c9ed273f@app.fastmail.com/
Link: https://pastebin.com/3SDaH8C6
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Boris Burkov <boris@bur.io>
Signed-off-by: David Sterba <dsterba@suse.com>
---
Changelog:
v3:
- handle BTRFS_IOERR set on the ordered_extent in btrfs_dio_iomap_end.
  If the bio fails before we loop in the submission loop and exit from
  the loop early, we never submit a second bio covering the rest of the
  extent range, resulting in leaking the ordered_extent, which hangs umount.
  We can distinguish this from a short write in btrfs_dio_iomap_end by
  checking the ordered_extent.
v2:
- rename new ordered extent function
- pull the new function into a prep patch
- reorganize how the ordered_extent is stored/passed around to avoid so
many annoying memsets and exposing it to fs/btrfs/file.c
- lots of small code style improvements
- remove unintentional whitespace changes
- commit message improvements
- various ASSERTs for clarity/debugging

 fs/btrfs/btrfs_inode.h |  1 +
 fs/btrfs/file.c        | 11 +++++-
 fs/btrfs/inode.c       | 76 +++++++++++++++++++++++++++++++-----------
 3 files changed, 68 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 49a92aa65de1..87020aa58121 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -516,6 +516,7 @@ ssize_t btrfs_do_encoded_write(struct kiocb *iocb, struct iov_iter *from,
 ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter,
 		       size_t done_before);
 struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter,
+				  struct btrfs_ordered_extent **ordered_extent,
 				  size_t done_before);
 
 extern const struct dentry_operations btrfs_dentry_operations;
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 5cc5a1faaef5..ec5c5355906b 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1465,6 +1465,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t err;
 	unsigned int ilock_flags = 0;
 	struct iomap_dio *dio;
+	struct btrfs_ordered_extent *ordered_extent = NULL;
 
 	if (iocb->ki_flags & IOCB_NOWAIT)
 		ilock_flags |= BTRFS_ILOCK_TRY;
@@ -1526,7 +1527,7 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	 * got -EFAULT, faulting in the pages before the retry.
 	 */
 	from->nofault = true;
-	dio = btrfs_dio_write(iocb, from, written);
+	dio = btrfs_dio_write(iocb, from, &ordered_extent, written);
 	from->nofault = false;
 
 	/*
@@ -1569,6 +1570,14 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)
 			goto relock;
 		}
 	}
+	/*
+	 * We can't loop back to btrfs_dio_write, so we can drop the cached
+	 * ordered extent. Typically btrfs_dio_iomap_end will run and put the
+	 * ordered_extent, but this is needed to clean up in case of an error
+	 * path breaking out of iomap_iter before the final iomap_end call.
+	 */
+	if (ordered_extent)
+		btrfs_put_ordered_extent(ordered_extent);
 
 	/*
 	 * If 'err' is -ENOTBLK or we have not written all data, then it means
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8f07d59e8193..94f31697a278 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -81,6 +81,7 @@ struct btrfs_dio_data {
 	struct extent_changeset *data_reserved;
 	bool data_space_reserved;
 	bool nocow_done;
+	struct btrfs_ordered_extent *ordered;
 };
 
 struct btrfs_dio_private {
@@ -6963,6 +6964,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode,
 }
 
 static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
+						  struct btrfs_dio_data *dio_data,
 						  const u64 start,
 						  const u64 len,
 						  const u64 orig_start,
@@ -6973,7 +6975,7 @@ static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
 						  const int type)
 {
 	struct extent_map *em = NULL;
-	int ret;
+	struct btrfs_ordered_extent *ordered;
 
 	if (type != BTRFS_ORDERED_NOCOW) {
 		em = create_io_em(inode, start, len, orig_start, block_start,
@@ -6983,18 +6985,21 @@ static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
 		if (IS_ERR(em))
 			goto out;
 	}
-	ret = btrfs_add_ordered_extent(inode, start, len, len, block_start,
-				       block_len, 0,
-				       (1 << type) |
-				       (1 << BTRFS_ORDERED_DIRECT),
-				       BTRFS_COMPRESS_NONE);
-	if (ret) {
+	ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
+					     block_start, block_len, 0,
+					     (1 << type) |
+					     (1 << BTRFS_ORDERED_DIRECT),
+					     BTRFS_COMPRESS_NONE);
+	if (IS_ERR(ordered)) {
 		if (em) {
 			free_extent_map(em);
 			btrfs_drop_extent_map_range(inode, start,
 						    start + len - 1, false);
 		}
-		em = ERR_PTR(ret);
+		em = ERR_PTR(PTR_ERR(ordered));
+	} else {
+		ASSERT(!dio_data->ordered);
+		dio_data->ordered = ordered;
 	}
  out:
 
@@ -7002,6 +7007,7 @@ static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
 }
 
 static struct extent_map *btrfs_new_extent_direct(struct btrfs_inode *inode,
+						  struct btrfs_dio_data *dio_data,
 						  u64 start, u64 len)
 {
 	struct btrfs_root *root = inode->root;
@@ -7017,7 +7023,8 @@ static struct extent_map *btrfs_new_extent_direct(struct btrfs_inode *inode,
 	if (ret)
 		return ERR_PTR(ret);
 
-	em = btrfs_create_dio_extent(inode, start, ins.offset, start,
+	em = btrfs_create_dio_extent(inode, dio_data,
+				     start, ins.offset, start,
 				     ins.objectid, ins.offset, ins.offset,
 				     ins.offset, BTRFS_ORDERED_REGULAR);
 	btrfs_dec_block_group_reservations(fs_info, ins.objectid);
@@ -7362,7 +7369,7 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
 		}
 		space_reserved = true;
 
-		em2 = btrfs_create_dio_extent(BTRFS_I(inode), start, len,
+		em2 = btrfs_create_dio_extent(BTRFS_I(inode), dio_data, start, len,
 					      orig_start, block_start,
 					      len, orig_block_len,
 					      ram_bytes, type);
@@ -7404,7 +7411,7 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
 			goto out;
 		space_reserved = true;
 
-		em = btrfs_new_extent_direct(BTRFS_I(inode), start, len);
+		em = btrfs_new_extent_direct(BTRFS_I(inode), dio_data, start, len);
 		if (IS_ERR(em)) {
 			ret = PTR_ERR(em);
 			goto out;
@@ -7508,6 +7515,17 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 		}
 	}
 
+	if (dio_data->ordered) {
+		ASSERT(write);
+		em = btrfs_get_extent(BTRFS_I(inode), NULL, 0,
+				      dio_data->ordered->file_offset,
+				      dio_data->ordered->bytes_left);
+		if (IS_ERR(em)) {
+			ret = PTR_ERR(em);
+			goto err;
+		}
+		goto map_iomap;
+	}
 	memset(dio_data, 0, sizeof(*dio_data));
 
 	/*
@@ -7649,6 +7667,7 @@ static int btrfs_dio_iomap_begin(struct inode *inode, loff_t start,
 	else
 		free_extent_state(cached_state);
 
+map_iomap:
 	/*
 	 * Translate extent map information to iomap.
 	 * We trim the extents (and move the addr) even though iomap code does
@@ -7702,13 +7721,27 @@ static int btrfs_dio_iomap_end(struct inode *inode, loff_t pos, loff_t length,
 	if (submitted < length) {
 		pos += submitted;
 		length -= submitted;
-		if (write)
-			btrfs_mark_ordered_io_finished(BTRFS_I(inode), NULL,
-						       pos, length, false);
-		else
+		if (write) {
+			struct btrfs_ordered_extent *ordered = dio_data->ordered;
+			if (submitted == 0 || 
+			    test_bit(BTRFS_ORDERED_IOERR, &ordered->flags)) {
+				btrfs_mark_ordered_io_finished(BTRFS_I(inode),
+							       NULL, pos,
+							       length, false);
+				btrfs_put_ordered_extent(ordered);
+				dio_data->ordered = NULL;
+			}
+		} else {
 			unlock_extent(&BTRFS_I(inode)->io_tree, pos,
 				      pos + length - 1, NULL);
+		}
 		ret = -ENOTBLK;
+	} else {
+		/* On the last bio, release our cached ordered_extent. */
+		if (write) {
+			btrfs_put_ordered_extent(dio_data->ordered);
+			dio_data->ordered = NULL;
+		}
 	}
 
 	if (write)
@@ -7771,19 +7804,24 @@ static const struct iomap_dio_ops btrfs_dio_ops = {
 
 ssize_t btrfs_dio_read(struct kiocb *iocb, struct iov_iter *iter, size_t done_before)
 {
-	struct btrfs_dio_data data;
+	struct btrfs_dio_data data = { 0 };
 
 	return iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops,
 			    IOMAP_DIO_PARTIAL, &data, done_before);
 }
 
 struct iomap_dio *btrfs_dio_write(struct kiocb *iocb, struct iov_iter *iter,
+				  struct btrfs_ordered_extent **ordered_extent,
 				  size_t done_before)
 {
-	struct btrfs_dio_data data;
+	struct btrfs_dio_data dio_data = { .ordered = *ordered_extent };
+	struct iomap_dio *dio;
 
-	return __iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops,
-			    IOMAP_DIO_PARTIAL, &data, done_before);
+	dio =  __iomap_dio_rw(iocb, iter, &btrfs_dio_iomap_ops, &btrfs_dio_ops,
+			      IOMAP_DIO_PARTIAL, &dio_data, done_before);
+	if (!IS_ERR_OR_NULL(dio))
+		*ordered_extent = dio_data.ordered;
+	return dio;
 }
 
 static int btrfs_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
-- 
2.38.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-07 20:49 [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault Boris Burkov
@ 2023-03-07 23:47 ` kernel test robot
  2023-03-08 16:00 ` Sweet Tea Dorminy
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-03-07 23:47 UTC (permalink / raw)
  To: Boris Burkov; +Cc: oe-kbuild-all

Hi Boris,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on linus/master v6.3-rc1]
[cannot apply to next-20230307]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/6733f2fac24b674d9f60dc1093de30513c099629.1678212067.git.boris%40bur.io
patch subject: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230308/202303080727.U1vbreEf-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/938364315c6a1c0055382359a2d94d4d069a273c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
        git checkout 938364315c6a1c0055382359a2d94d4d069a273c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303080727.U1vbreEf-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/btrfs/inode.c: In function 'btrfs_create_dio_extent':
   fs/btrfs/inode.c:7001:19: error: implicit declaration of function 'btrfs_alloc_ordered_extent'; did you mean 'btrfs_split_ordered_extent'? [-Werror=implicit-function-declaration]
    7001 |         ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |                   btrfs_split_ordered_extent
>> fs/btrfs/inode.c:7001:17: warning: assignment to 'struct btrfs_ordered_extent *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    7001 |         ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
         |                 ^
   cc1: some warnings being treated as errors


vim +7001 fs/btrfs/inode.c

  6978	
  6979	static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
  6980							  struct btrfs_dio_data *dio_data,
  6981							  const u64 start,
  6982							  const u64 len,
  6983							  const u64 orig_start,
  6984							  const u64 block_start,
  6985							  const u64 block_len,
  6986							  const u64 orig_block_len,
  6987							  const u64 ram_bytes,
  6988							  const int type)
  6989	{
  6990		struct extent_map *em = NULL;
  6991		struct btrfs_ordered_extent *ordered;
  6992	
  6993		if (type != BTRFS_ORDERED_NOCOW) {
  6994			em = create_io_em(inode, start, len, orig_start, block_start,
  6995					  block_len, orig_block_len, ram_bytes,
  6996					  BTRFS_COMPRESS_NONE, /* compress_type */
  6997					  type);
  6998			if (IS_ERR(em))
  6999				goto out;
  7000		}
> 7001		ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
  7002						     block_start, block_len, 0,
  7003						     (1 << type) |
  7004						     (1 << BTRFS_ORDERED_DIRECT),
  7005						     BTRFS_COMPRESS_NONE);
  7006		if (IS_ERR(ordered)) {
  7007			if (em) {
  7008				free_extent_map(em);
  7009				btrfs_drop_extent_map_range(inode, start,
  7010							    start + len - 1, false);
  7011			}
  7012			em = ERR_PTR(PTR_ERR(ordered));
  7013		} else {
  7014			ASSERT(!dio_data->ordered);
  7015			dio_data->ordered = ordered;
  7016		}
  7017	 out:
  7018	
  7019		return em;
  7020	}
  7021	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-07 20:49 [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault Boris Burkov
  2023-03-07 23:47 ` kernel test robot
@ 2023-03-08 16:00 ` Sweet Tea Dorminy
  2023-03-09  7:36 ` kernel test robot
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: Sweet Tea Dorminy @ 2023-03-08 16:00 UTC (permalink / raw)
  To: Boris Burkov, linux-btrfs, kernel-team



On 3/7/23 15:49, Boris Burkov wrote:
> If an application is doing direct io to a btrfs file and experiences a
> page fault reading from the write buffer, iomap will issue a partial
> bio, and allow the fs to keep going. However, there was a subtle bug in
> this code path in the btrfs dio iomap implementation that led to the
> partial write ending up as a gap in the file's extents and to be read
> back as zeros.
> 
> The sequence of events in a partial write, lightly summarized and
> trimmed down for brevity is as follows:
> 
>    ====WRITING TASK====
>    btrfs_direct_write
>    __iomap_dio_write
>    iomap_iter
>      btrfs_dio_iomap_begin # create full ordered extent
>    iomap_dio_bio_iter
>      bio_iov_iter_get_pages # page fault; partial read
>      submit_bio # partial bio
>    iomap_iter
>      btrfs_dio_iomap_end
>        btrfs_mark_ordered_io_finished # sets BTRFS_ORDERED_IOERR;
> 				     # submit to finish_ordered_fn wq
>    fault_in_iov_iter_readable # btrfs_direct_write detects partial write
>    __iomap_dio_write
>    iomap_iter
>      btrfs_dio_iomap_begin # create second partial ordered extent
>    iomap_dio_bio_iter
>      bio_iov_iter_get_pages # read all of remainder
>      submit_bio # partial bio with all of remainder
>    iomap_iter
>      btrfs_dio_iomap_end # nothing exciting to do with ordered io
> 
>    ====DIO ENDIO====
>    ==FIRST PARTIAL BIO==
>    btrfs_dio_end_io
>      btrfs_mark_ordered_io_finished # bytes_left > 0
> 				   # don't submit to finish_ordered_fn wq
>    ==SECOND PARTIAL BIO==
>    btrfs_dio_end_io
>      btrfs_mark_ordered_io_finished # bytes_left == 0
> 				   # submit to finish_ordered_fn wq
> 
>    ====BTRFS FINISH ORDERED WQ====
>    ==FIRST PARTIAL BIO==
>    btrfs_finish_ordered_io # called by dio_iomap_end_io, sees
> 			  # BTRFS_ORDERED_IOERR, just drops the
> 			  # ordered_extent
>    ==SECOND PARTIAL BIO==
>    btrfs_finish_ordered_io # called by btrfs_dio_end_io, writes out file
> 			  # extents, csums, etc...
> 
> The essence of the problem is that while btrfs_direct_write and iomap
> properly interact to submit all the correct bios, there is insufficient
> logic in the btrfs dio functions (btrfs_dio_iomap_begin,
> btrfs_dio_submit_io, btrfs_dio_end_io, and btrfs_dio_iomap_end) to
> ensure that every bio is at least a part of a completed ordered_extent.
> And it is completing an ordered_extent that results in crucial
> functionality like inserting a file extent item for the range in the
> subvolume/fs tree.
> 
> More specifically, btrfs_dio_end_io treats the ordered extent as
> unfinished but btrfs_dio_iomap_end sets BTRFS_ORDERED_IOERR on it.
> Thus, the finish io work doesn't result in file extents, csums, etc...
> In the aftermath, such a file behaves as though it has a hole in it,
> instead of the purportedly written data.
> 
> We considered a few options for fixing the bug:
> 
> 1. treat the partial bio as if we had truncated the file, which would
>     result in properly finishing it.
> 2. split the ordered extent when submitting a partial bio.
> 3. cache the ordered extent across calls to __iomap_dio_rw in
>     iter->private, so that we could reuse it and correctly apply several
>     bios to it.
> 
> I had trouble with 1, and it felt the most like a hack, so I tried 2
> and 3. Since 3 has the benefit of also not creating an extra file
> extent, and avoids an ordered extent lookup during bio submission, it
> felt like the best option.
> 
> A quick summary of the changes necessary to implement this cached
> ordered_extent behavior:
> 
> - btrfs_direct_write keeps track of an ordered_extent for the duration
>    of a call, possible across several __iomap_dio_rws.
> - zero the btrfs_dio_data before using it, since its fields constitute
>    state now.
> - btrfs_dio_write uses dio_data to pass this ordered extent into and out
>    of __iomap_dio_rw.
> - when the write is done, put the ordered_extent.
> - if the short write happens to be length 0, then we _don't_ get an
>    extra bio, so we do need to cancel the ordered_extent like we used
>    to (and ditch the cached ordered extent)
> - if the short write ordered_extent has an error on it, drop the cached
>    ordered extent, as before.
> - in btrfs_dio_iomap_begin, if the cached ordered extent is present,
>    skip all the work of creating it, just look up the extent mapping and
>    jump to setting up the iomap. (This part could likely be more
>    elegant..)
> 
> Thanks to Josef, Christoph, and Filipe with their help figuring out the
> bug and the fix.
> 
> Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2169947
> Link: https://lore.kernel.org/linux-btrfs/aa1fb69e-b613-47aa-a99e-a0a2c9ed273f@app.fastmail.com/
> Link: https://pastebin.com/3SDaH8C6
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Boris Burkov <boris@bur.io>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> Changelog:
> v3:
> - handle BTRFS_IOERR set on the ordered_extent in btrfs_dio_iomap_end.
>    If the bio fails before we loop in the submission loop and exit from
>    the loop early, we never submit a second bio covering the rest of the
>    extent range, resulting in leaking the ordered_extent, which hangs umount.
>    We can distinguish this from a short write in btrfs_dio_iomap_end by
>    checking the ordered_extent.
> v2:
> - rename new ordered extent function
> - pull the new function into a prep patch
> - reorganize how the ordered_extent is stored/passed around to avoid so
> many annoying memsets and exposing it to fs/btrfs/file.c
> - lots of small code style improvements
> - remove unintentional whitespace changes
> - commit message improvements
> - various ASSERTs for clarity/debugging
> 
>   fs/btrfs/btrfs_inode.h |  1 +
>   fs/btrfs/file.c        | 11 +++++-
>   fs/btrfs/inode.c       | 76 +++++++++++++++++++++++++++++++-----------
>   3 files changed, 68 insertions(+), 20 deletions(-)
> 

Ran generic/250 and generic/276 each 7500 times on this patchset, with 
no failures; previously I'd hit the race condition in each after 10-50 
times.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-07 20:49 [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault Boris Burkov
  2023-03-07 23:47 ` kernel test robot
  2023-03-08 16:00 ` Sweet Tea Dorminy
@ 2023-03-09  7:36 ` kernel test robot
  2023-03-09  7:46 ` kernel test robot
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-03-09  7:36 UTC (permalink / raw)
  To: Boris Burkov; +Cc: llvm, oe-kbuild-all

Hi Boris,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on linus/master v6.3-rc1 next-20230309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/6733f2fac24b674d9f60dc1093de30513c099629.1678212067.git.boris%40bur.io
patch subject: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
config: riscv-randconfig-r042-20230308 (https://download.01.org/0day-ci/archive/20230309/202303091543.12BtMYMB-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/938364315c6a1c0055382359a2d94d4d069a273c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
        git checkout 938364315c6a1c0055382359a2d94d4d069a273c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash fs/btrfs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303091543.12BtMYMB-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/btrfs/inode.c:7001:12: error: call to undeclared function 'btrfs_alloc_ordered_extent'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
           ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
                     ^
>> fs/btrfs/inode.c:7001:10: error: incompatible integer to pointer conversion assigning to 'struct btrfs_ordered_extent *' from 'int' [-Wint-conversion]
           ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
                   ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 errors generated.


vim +/btrfs_alloc_ordered_extent +7001 fs/btrfs/inode.c

  6978	
  6979	static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
  6980							  struct btrfs_dio_data *dio_data,
  6981							  const u64 start,
  6982							  const u64 len,
  6983							  const u64 orig_start,
  6984							  const u64 block_start,
  6985							  const u64 block_len,
  6986							  const u64 orig_block_len,
  6987							  const u64 ram_bytes,
  6988							  const int type)
  6989	{
  6990		struct extent_map *em = NULL;
  6991		struct btrfs_ordered_extent *ordered;
  6992	
  6993		if (type != BTRFS_ORDERED_NOCOW) {
  6994			em = create_io_em(inode, start, len, orig_start, block_start,
  6995					  block_len, orig_block_len, ram_bytes,
  6996					  BTRFS_COMPRESS_NONE, /* compress_type */
  6997					  type);
  6998			if (IS_ERR(em))
  6999				goto out;
  7000		}
> 7001		ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
  7002						     block_start, block_len, 0,
  7003						     (1 << type) |
  7004						     (1 << BTRFS_ORDERED_DIRECT),
  7005						     BTRFS_COMPRESS_NONE);
  7006		if (IS_ERR(ordered)) {
  7007			if (em) {
  7008				free_extent_map(em);
  7009				btrfs_drop_extent_map_range(inode, start,
  7010							    start + len - 1, false);
  7011			}
  7012			em = ERR_PTR(PTR_ERR(ordered));
  7013		} else {
  7014			ASSERT(!dio_data->ordered);
  7015			dio_data->ordered = ordered;
  7016		}
  7017	 out:
  7018	
  7019		return em;
  7020	}
  7021	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-07 20:49 [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault Boris Burkov
                   ` (2 preceding siblings ...)
  2023-03-09  7:36 ` kernel test robot
@ 2023-03-09  7:46 ` kernel test robot
  2023-03-09  8:39 ` kernel test robot
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-03-09  7:46 UTC (permalink / raw)
  To: Boris Burkov; +Cc: oe-kbuild-all

Hi Boris,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on linus/master v6.3-rc1 next-20230309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/6733f2fac24b674d9f60dc1093de30513c099629.1678212067.git.boris%40bur.io
patch subject: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
config: arc-randconfig-r043-20230308 (https://download.01.org/0day-ci/archive/20230309/202303091501.3JopRswb-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/938364315c6a1c0055382359a2d94d4d069a273c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
        git checkout 938364315c6a1c0055382359a2d94d4d069a273c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash fs/btrfs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303091501.3JopRswb-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   fs/btrfs/inode.c: In function 'btrfs_create_dio_extent':
>> fs/btrfs/inode.c:7001:19: error: implicit declaration of function 'btrfs_alloc_ordered_extent'; did you mean 'btrfs_split_ordered_extent'? [-Werror=implicit-function-declaration]
    7001 |         ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |                   btrfs_split_ordered_extent
>> fs/btrfs/inode.c:7001:17: warning: assignment to 'struct btrfs_ordered_extent *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
    7001 |         ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
         |                 ^
   cc1: some warnings being treated as errors


vim +7001 fs/btrfs/inode.c

  6978	
  6979	static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
  6980							  struct btrfs_dio_data *dio_data,
  6981							  const u64 start,
  6982							  const u64 len,
  6983							  const u64 orig_start,
  6984							  const u64 block_start,
  6985							  const u64 block_len,
  6986							  const u64 orig_block_len,
  6987							  const u64 ram_bytes,
  6988							  const int type)
  6989	{
  6990		struct extent_map *em = NULL;
  6991		struct btrfs_ordered_extent *ordered;
  6992	
  6993		if (type != BTRFS_ORDERED_NOCOW) {
  6994			em = create_io_em(inode, start, len, orig_start, block_start,
  6995					  block_len, orig_block_len, ram_bytes,
  6996					  BTRFS_COMPRESS_NONE, /* compress_type */
  6997					  type);
  6998			if (IS_ERR(em))
  6999				goto out;
  7000		}
> 7001		ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
  7002						     block_start, block_len, 0,
  7003						     (1 << type) |
  7004						     (1 << BTRFS_ORDERED_DIRECT),
  7005						     BTRFS_COMPRESS_NONE);
  7006		if (IS_ERR(ordered)) {
  7007			if (em) {
  7008				free_extent_map(em);
  7009				btrfs_drop_extent_map_range(inode, start,
  7010							    start + len - 1, false);
  7011			}
  7012			em = ERR_PTR(PTR_ERR(ordered));
  7013		} else {
  7014			ASSERT(!dio_data->ordered);
  7015			dio_data->ordered = ordered;
  7016		}
  7017	 out:
  7018	
  7019		return em;
  7020	}
  7021	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-07 20:49 [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault Boris Burkov
                   ` (3 preceding siblings ...)
  2023-03-09  7:46 ` kernel test robot
@ 2023-03-09  8:39 ` kernel test robot
  2023-03-09 10:43 ` kernel test robot
  2023-03-09 17:40 ` David Sterba
  6 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-03-09  8:39 UTC (permalink / raw)
  To: Boris Burkov; +Cc: llvm, oe-kbuild-all

Hi Boris,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on linus/master v6.3-rc1 next-20230309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/6733f2fac24b674d9f60dc1093de30513c099629.1678212067.git.boris%40bur.io
patch subject: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
config: x86_64-randconfig-a003 (https://download.01.org/0day-ci/archive/20230309/202303091632.k53VZjyD-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/938364315c6a1c0055382359a2d94d4d069a273c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
        git checkout 938364315c6a1c0055382359a2d94d4d069a273c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/btrfs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303091632.k53VZjyD-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/btrfs/inode.c:7001:12: error: implicit declaration of function 'btrfs_alloc_ordered_extent' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
                     ^
>> fs/btrfs/inode.c:7001:10: warning: incompatible integer to pointer conversion assigning to 'struct btrfs_ordered_extent *' from 'int' [-Wint-conversion]
           ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
                   ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 1 error generated.


vim +7001 fs/btrfs/inode.c

  6978	
  6979	static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
  6980							  struct btrfs_dio_data *dio_data,
  6981							  const u64 start,
  6982							  const u64 len,
  6983							  const u64 orig_start,
  6984							  const u64 block_start,
  6985							  const u64 block_len,
  6986							  const u64 orig_block_len,
  6987							  const u64 ram_bytes,
  6988							  const int type)
  6989	{
  6990		struct extent_map *em = NULL;
  6991		struct btrfs_ordered_extent *ordered;
  6992	
  6993		if (type != BTRFS_ORDERED_NOCOW) {
  6994			em = create_io_em(inode, start, len, orig_start, block_start,
  6995					  block_len, orig_block_len, ram_bytes,
  6996					  BTRFS_COMPRESS_NONE, /* compress_type */
  6997					  type);
  6998			if (IS_ERR(em))
  6999				goto out;
  7000		}
> 7001		ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
  7002						     block_start, block_len, 0,
  7003						     (1 << type) |
  7004						     (1 << BTRFS_ORDERED_DIRECT),
  7005						     BTRFS_COMPRESS_NONE);
  7006		if (IS_ERR(ordered)) {
  7007			if (em) {
  7008				free_extent_map(em);
  7009				btrfs_drop_extent_map_range(inode, start,
  7010							    start + len - 1, false);
  7011			}
  7012			em = ERR_PTR(PTR_ERR(ordered));
  7013		} else {
  7014			ASSERT(!dio_data->ordered);
  7015			dio_data->ordered = ordered;
  7016		}
  7017	 out:
  7018	
  7019		return em;
  7020	}
  7021	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-07 20:49 [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault Boris Burkov
                   ` (4 preceding siblings ...)
  2023-03-09  8:39 ` kernel test robot
@ 2023-03-09 10:43 ` kernel test robot
  2023-03-09 17:40 ` David Sterba
  6 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2023-03-09 10:43 UTC (permalink / raw)
  To: Boris Burkov; +Cc: llvm, oe-kbuild-all

Hi Boris,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on linus/master v6.3-rc1 next-20230309]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/6733f2fac24b674d9f60dc1093de30513c099629.1678212067.git.boris%40bur.io
patch subject: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
config: x86_64-randconfig-a003 (https://download.01.org/0day-ci/archive/20230309/202303091846.eSPpkHU6-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/938364315c6a1c0055382359a2d94d4d069a273c
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Boris-Burkov/btrfs-fix-dio-continue-after-short-write-due-to-buffer-page-fault/20230308-044956
        git checkout 938364315c6a1c0055382359a2d94d4d069a273c
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303091846.eSPpkHU6-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/btrfs/inode.c:7001:12: error: implicit declaration of function 'btrfs_alloc_ordered_extent' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
           ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
                     ^
   fs/btrfs/inode.c:7001:10: warning: incompatible integer to pointer conversion assigning to 'struct btrfs_ordered_extent *' from 'int' [-Wint-conversion]
           ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
                   ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 1 error generated.


vim +/btrfs_alloc_ordered_extent +7001 fs/btrfs/inode.c

  6978	
  6979	static struct extent_map *btrfs_create_dio_extent(struct btrfs_inode *inode,
  6980							  struct btrfs_dio_data *dio_data,
  6981							  const u64 start,
  6982							  const u64 len,
  6983							  const u64 orig_start,
  6984							  const u64 block_start,
  6985							  const u64 block_len,
  6986							  const u64 orig_block_len,
  6987							  const u64 ram_bytes,
  6988							  const int type)
  6989	{
  6990		struct extent_map *em = NULL;
  6991		struct btrfs_ordered_extent *ordered;
  6992	
  6993		if (type != BTRFS_ORDERED_NOCOW) {
  6994			em = create_io_em(inode, start, len, orig_start, block_start,
  6995					  block_len, orig_block_len, ram_bytes,
  6996					  BTRFS_COMPRESS_NONE, /* compress_type */
  6997					  type);
  6998			if (IS_ERR(em))
  6999				goto out;
  7000		}
> 7001		ordered = btrfs_alloc_ordered_extent(inode, start, len, len,
  7002						     block_start, block_len, 0,
  7003						     (1 << type) |
  7004						     (1 << BTRFS_ORDERED_DIRECT),
  7005						     BTRFS_COMPRESS_NONE);
  7006		if (IS_ERR(ordered)) {
  7007			if (em) {
  7008				free_extent_map(em);
  7009				btrfs_drop_extent_map_range(inode, start,
  7010							    start + len - 1, false);
  7011			}
  7012			em = ERR_PTR(PTR_ERR(ordered));
  7013		} else {
  7014			ASSERT(!dio_data->ordered);
  7015			dio_data->ordered = ordered;
  7016		}
  7017	 out:
  7018	
  7019		return em;
  7020	}
  7021	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-07 20:49 [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault Boris Burkov
                   ` (5 preceding siblings ...)
  2023-03-09 10:43 ` kernel test robot
@ 2023-03-09 17:40 ` David Sterba
  2023-03-15 19:53   ` Boris Burkov
  6 siblings, 1 reply; 10+ messages in thread
From: David Sterba @ 2023-03-09 17:40 UTC (permalink / raw)
  To: Boris Burkov; +Cc: linux-btrfs, kernel-team

On Tue, Mar 07, 2023 at 12:49:30PM -0800, Boris Burkov wrote:
> If an application is doing direct io to a btrfs file and experiences a
> page fault reading from the write buffer, iomap will issue a partial
> bio, and allow the fs to keep going. However, there was a subtle bug in
> this code path in the btrfs dio iomap implementation that led to the
> partial write ending up as a gap in the file's extents and to be read
> back as zeros.
> 
> The sequence of events in a partial write, lightly summarized and
> trimmed down for brevity is as follows:
> 
>   ====WRITING TASK====
>   btrfs_direct_write
>   __iomap_dio_write
>   iomap_iter
>     btrfs_dio_iomap_begin # create full ordered extent
>   iomap_dio_bio_iter
>     bio_iov_iter_get_pages # page fault; partial read
>     submit_bio # partial bio
>   iomap_iter
>     btrfs_dio_iomap_end
>       btrfs_mark_ordered_io_finished # sets BTRFS_ORDERED_IOERR;
> 				     # submit to finish_ordered_fn wq
>   fault_in_iov_iter_readable # btrfs_direct_write detects partial write
>   __iomap_dio_write
>   iomap_iter
>     btrfs_dio_iomap_begin # create second partial ordered extent
>   iomap_dio_bio_iter
>     bio_iov_iter_get_pages # read all of remainder
>     submit_bio # partial bio with all of remainder
>   iomap_iter
>     btrfs_dio_iomap_end # nothing exciting to do with ordered io
> 
>   ====DIO ENDIO====
>   ==FIRST PARTIAL BIO==
>   btrfs_dio_end_io
>     btrfs_mark_ordered_io_finished # bytes_left > 0
> 				   # don't submit to finish_ordered_fn wq
>   ==SECOND PARTIAL BIO==
>   btrfs_dio_end_io
>     btrfs_mark_ordered_io_finished # bytes_left == 0
> 				   # submit to finish_ordered_fn wq
> 
>   ====BTRFS FINISH ORDERED WQ====
>   ==FIRST PARTIAL BIO==
>   btrfs_finish_ordered_io # called by dio_iomap_end_io, sees
> 			  # BTRFS_ORDERED_IOERR, just drops the
> 			  # ordered_extent
>   ==SECOND PARTIAL BIO==
>   btrfs_finish_ordered_io # called by btrfs_dio_end_io, writes out file
> 			  # extents, csums, etc...
> 
> The essence of the problem is that while btrfs_direct_write and iomap
> properly interact to submit all the correct bios, there is insufficient
> logic in the btrfs dio functions (btrfs_dio_iomap_begin,
> btrfs_dio_submit_io, btrfs_dio_end_io, and btrfs_dio_iomap_end) to
> ensure that every bio is at least a part of a completed ordered_extent.
> And it is completing an ordered_extent that results in crucial
> functionality like inserting a file extent item for the range in the
> subvolume/fs tree.
> 
> More specifically, btrfs_dio_end_io treats the ordered extent as
> unfinished but btrfs_dio_iomap_end sets BTRFS_ORDERED_IOERR on it.
> Thus, the finish io work doesn't result in file extents, csums, etc...
> In the aftermath, such a file behaves as though it has a hole in it,
> instead of the purportedly written data.
> 
> We considered a few options for fixing the bug:
> 
> 1. treat the partial bio as if we had truncated the file, which would
>    result in properly finishing it.
> 2. split the ordered extent when submitting a partial bio.
> 3. cache the ordered extent across calls to __iomap_dio_rw in
>    iter->private, so that we could reuse it and correctly apply several
>    bios to it.
> 
> I had trouble with 1, and it felt the most like a hack, so I tried 2
> and 3. Since 3 has the benefit of also not creating an extra file
> extent, and avoids an ordered extent lookup during bio submission, it
> felt like the best option.
> 
> A quick summary of the changes necessary to implement this cached
> ordered_extent behavior:
> 
> - btrfs_direct_write keeps track of an ordered_extent for the duration
>   of a call, possible across several __iomap_dio_rws.
> - zero the btrfs_dio_data before using it, since its fields constitute
>   state now.
> - btrfs_dio_write uses dio_data to pass this ordered extent into and out
>   of __iomap_dio_rw.
> - when the write is done, put the ordered_extent.
> - if the short write happens to be length 0, then we _don't_ get an
>   extra bio, so we do need to cancel the ordered_extent like we used
>   to (and ditch the cached ordered extent)
> - if the short write ordered_extent has an error on it, drop the cached
>   ordered extent, as before.
> - in btrfs_dio_iomap_begin, if the cached ordered extent is present,
>   skip all the work of creating it, just look up the extent mapping and
>   jump to setting up the iomap. (This part could likely be more
>   elegant..)
> 
> Thanks to Josef, Christoph, and Filipe with their help figuring out the
> bug and the fix.
> 
> Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes")
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2169947
> Link: https://lore.kernel.org/linux-btrfs/aa1fb69e-b613-47aa-a99e-a0a2c9ed273f@app.fastmail.com/
> Link: https://pastebin.com/3SDaH8C6
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Boris Burkov <boris@bur.io>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> Changelog:
> v3:
> - handle BTRFS_IOERR set on the ordered_extent in btrfs_dio_iomap_end.
>   If the bio fails before we loop in the submission loop and exit from
>   the loop early, we never submit a second bio covering the rest of the
>   extent range, resulting in leaking the ordered_extent, which hangs umount.
>   We can distinguish this from a short write in btrfs_dio_iomap_end by
>   checking the ordered_extent.

Replaced v2 in misc-next, thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-15 19:53   ` Boris Burkov
@ 2023-03-15 19:52     ` David Sterba
  0 siblings, 0 replies; 10+ messages in thread
From: David Sterba @ 2023-03-15 19:52 UTC (permalink / raw)
  To: Boris Burkov; +Cc: David Sterba, linux-btrfs, kernel-team

On Wed, Mar 15, 2023 at 12:53:24PM -0700, Boris Burkov wrote:
> On Thu, Mar 09, 2023 at 06:40:50PM +0100, David Sterba wrote:
> > On Tue, Mar 07, 2023 at 12:49:30PM -0800, Boris Burkov wrote:
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2169947
> > > Link: https://lore.kernel.org/linux-btrfs/aa1fb69e-b613-47aa-a99e-a0a2c9ed273f@app.fastmail.com/
> > > Link: https://pastebin.com/3SDaH8C6
> > > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > Signed-off-by: Boris Burkov <boris@bur.io>
> > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > ---
> > > Changelog:
> > > v3:
> > > - handle BTRFS_IOERR set on the ordered_extent in btrfs_dio_iomap_end.
> > >   If the bio fails before we loop in the submission loop and exit from
> > >   the loop early, we never submit a second bio covering the rest of the
> > >   extent range, resulting in leaking the ordered_extent, which hangs umount.
> > >   We can distinguish this from a short write in btrfs_dio_iomap_end by
> > >   checking the ordered_extent.
> > 
> > Replaced v2 in misc-next, thanks.
> 
> Embarassingly, I have now proven that this version is _still_ broken :(
> 
> TL;DR, this "fix" reintroduces the original bug Filipe was fixing in the
> first place, just a more subtle version of it.

Ok thanks, I'll remove the patch from branches.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault
  2023-03-09 17:40 ` David Sterba
@ 2023-03-15 19:53   ` Boris Burkov
  2023-03-15 19:52     ` David Sterba
  0 siblings, 1 reply; 10+ messages in thread
From: Boris Burkov @ 2023-03-15 19:53 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs, kernel-team

On Thu, Mar 09, 2023 at 06:40:50PM +0100, David Sterba wrote:
> On Tue, Mar 07, 2023 at 12:49:30PM -0800, Boris Burkov wrote:
> > If an application is doing direct io to a btrfs file and experiences a
> > page fault reading from the write buffer, iomap will issue a partial
> > bio, and allow the fs to keep going. However, there was a subtle bug in
> > this code path in the btrfs dio iomap implementation that led to the
> > partial write ending up as a gap in the file's extents and to be read
> > back as zeros.
> > 
> > The sequence of events in a partial write, lightly summarized and
> > trimmed down for brevity is as follows:
> > 
> >   ====WRITING TASK====
> >   btrfs_direct_write
> >   __iomap_dio_write
> >   iomap_iter
> >     btrfs_dio_iomap_begin # create full ordered extent
> >   iomap_dio_bio_iter
> >     bio_iov_iter_get_pages # page fault; partial read
> >     submit_bio # partial bio
> >   iomap_iter
> >     btrfs_dio_iomap_end
> >       btrfs_mark_ordered_io_finished # sets BTRFS_ORDERED_IOERR;
> > 				     # submit to finish_ordered_fn wq
> >   fault_in_iov_iter_readable # btrfs_direct_write detects partial write
> >   __iomap_dio_write
> >   iomap_iter
> >     btrfs_dio_iomap_begin # create second partial ordered extent
> >   iomap_dio_bio_iter
> >     bio_iov_iter_get_pages # read all of remainder
> >     submit_bio # partial bio with all of remainder
> >   iomap_iter
> >     btrfs_dio_iomap_end # nothing exciting to do with ordered io
> > 
> >   ====DIO ENDIO====
> >   ==FIRST PARTIAL BIO==
> >   btrfs_dio_end_io
> >     btrfs_mark_ordered_io_finished # bytes_left > 0
> > 				   # don't submit to finish_ordered_fn wq
> >   ==SECOND PARTIAL BIO==
> >   btrfs_dio_end_io
> >     btrfs_mark_ordered_io_finished # bytes_left == 0
> > 				   # submit to finish_ordered_fn wq
> > 
> >   ====BTRFS FINISH ORDERED WQ====
> >   ==FIRST PARTIAL BIO==
> >   btrfs_finish_ordered_io # called by dio_iomap_end_io, sees
> > 			  # BTRFS_ORDERED_IOERR, just drops the
> > 			  # ordered_extent
> >   ==SECOND PARTIAL BIO==
> >   btrfs_finish_ordered_io # called by btrfs_dio_end_io, writes out file
> > 			  # extents, csums, etc...
> > 
> > The essence of the problem is that while btrfs_direct_write and iomap
> > properly interact to submit all the correct bios, there is insufficient
> > logic in the btrfs dio functions (btrfs_dio_iomap_begin,
> > btrfs_dio_submit_io, btrfs_dio_end_io, and btrfs_dio_iomap_end) to
> > ensure that every bio is at least a part of a completed ordered_extent.
> > And it is completing an ordered_extent that results in crucial
> > functionality like inserting a file extent item for the range in the
> > subvolume/fs tree.
> > 
> > More specifically, btrfs_dio_end_io treats the ordered extent as
> > unfinished but btrfs_dio_iomap_end sets BTRFS_ORDERED_IOERR on it.
> > Thus, the finish io work doesn't result in file extents, csums, etc...
> > In the aftermath, such a file behaves as though it has a hole in it,
> > instead of the purportedly written data.
> > 
> > We considered a few options for fixing the bug:
> > 
> > 1. treat the partial bio as if we had truncated the file, which would
> >    result in properly finishing it.
> > 2. split the ordered extent when submitting a partial bio.
> > 3. cache the ordered extent across calls to __iomap_dio_rw in
> >    iter->private, so that we could reuse it and correctly apply several
> >    bios to it.
> > 
> > I had trouble with 1, and it felt the most like a hack, so I tried 2
> > and 3. Since 3 has the benefit of also not creating an extra file
> > extent, and avoids an ordered extent lookup during bio submission, it
> > felt like the best option.
> > 
> > A quick summary of the changes necessary to implement this cached
> > ordered_extent behavior:
> > 
> > - btrfs_direct_write keeps track of an ordered_extent for the duration
> >   of a call, possible across several __iomap_dio_rws.
> > - zero the btrfs_dio_data before using it, since its fields constitute
> >   state now.
> > - btrfs_dio_write uses dio_data to pass this ordered extent into and out
> >   of __iomap_dio_rw.
> > - when the write is done, put the ordered_extent.
> > - if the short write happens to be length 0, then we _don't_ get an
> >   extra bio, so we do need to cancel the ordered_extent like we used
> >   to (and ditch the cached ordered extent)
> > - if the short write ordered_extent has an error on it, drop the cached
> >   ordered extent, as before.
> > - in btrfs_dio_iomap_begin, if the cached ordered extent is present,
> >   skip all the work of creating it, just look up the extent mapping and
> >   jump to setting up the iomap. (This part could likely be more
> >   elegant..)
> > 
> > Thanks to Josef, Christoph, and Filipe with their help figuring out the
> > bug and the fix.
> > 
> > Fixes: 51bd9563b678 ("btrfs: fix deadlock due to page faults during direct IO reads and writes")
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2169947
> > Link: https://lore.kernel.org/linux-btrfs/aa1fb69e-b613-47aa-a99e-a0a2c9ed273f@app.fastmail.com/
> > Link: https://pastebin.com/3SDaH8C6
> > Reviewed-by: Filipe Manana <fdmanana@suse.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> > Changelog:
> > v3:
> > - handle BTRFS_IOERR set on the ordered_extent in btrfs_dio_iomap_end.
> >   If the bio fails before we loop in the submission loop and exit from
> >   the loop early, we never submit a second bio covering the rest of the
> >   extent range, resulting in leaking the ordered_extent, which hangs umount.
> >   We can distinguish this from a short write in btrfs_dio_iomap_end by
> >   checking the ordered_extent.
> 
> Replaced v2 in misc-next, thanks.

Embarassingly, I have now proven that this version is _still_ broken :(

TL;DR, this "fix" reintroduces the original bug Filipe was fixing in the
first place, just a more subtle version of it.

The original bug (some pseudocode liberties taken) was:
mmap(srcfile) = buf
open(srcfile, O_DIRECT|O_WRONLY|O_TRUNC) = fd
write(fd, buf, size=sz, off=0) = HANG!!!

and the issue was that iomap_begin created an OE, and then the page
fault handler did btrfs_readahead which blocked on the OE.

this led to the bug I was fixing, which was the case:
mmap(srcfile) = buf
open(dstfile, O_DIRECT|O_WRONLY|O_TRUNC) = fd
doStuff(buf); # partially in cache
write(fd, buf, size=sz, off=0) = CORRUPTION!!!

which we fixed by making the OE once again outlive the call into iomap,
and survive across multiple partial writes. HOWEVER, now we can trigger
the same original deadlock, we just have to work a bit harder.
mmap(srcfile) = buf
open(srcfile, O_DIRECT|O_WRONLY) = fd
doStuff(buf); # partially in cache
write(fd, buf, size=sz/2, off=sz/4) = HANG!!!

Crucially, we make two changes: we make the written region overlap with
the region backing the write buffer and we remove O_TRUNC. As a result,
we create an OE [sz/4, 3sz/4], do a partial write from the start of buf
(mapped to offset 0 in srcfile), then fault and attempt to fault in the
rest of srcfile while the OE exists, which is exactly the original
deadlock. Crucially, O_TRUNC and the exactly matching read/write regions
where covering this up by forcing the fault to always happen on the 0
byte, which got special handling, rather than a true partial fault.

With all that said, I think this strategy for fixing it, by keeping the
OE around is probably dead on arrival, and I need to scrap it and split
the ordered extent on a partial bio instead.

Sorry for the churn,
Boris

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-03-15 19:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-07 20:49 [PATCH v3] btrfs: fix dio continue after short write due to buffer page fault Boris Burkov
2023-03-07 23:47 ` kernel test robot
2023-03-08 16:00 ` Sweet Tea Dorminy
2023-03-09  7:36 ` kernel test robot
2023-03-09  7:46 ` kernel test robot
2023-03-09  8:39 ` kernel test robot
2023-03-09 10:43 ` kernel test robot
2023-03-09 17:40 ` David Sterba
2023-03-15 19:53   ` Boris Burkov
2023-03-15 19:52     ` David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.