linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings
@ 2017-08-23  6:45 Omar Sandoval
  2017-08-23  6:45 ` [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion Omar Sandoval
                   ` (6 more replies)
  0 siblings, 7 replies; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23  6:45 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

From: Omar Sandoval <osandov@fb.com>

I came across my ancient RCU string branch [1] and decided to revive it
and finally put it to rest. In the process of checking it with sparse, I
found a handful of other issues.

Patch 1 should probably go in 4.13, as it fixes bugs introduced this
cycle by the conversion to blk_status_t. Patch 2 is an old bug, so it
could wait for 4.14 but it might as well go in for 4.13. Patches 3-5 are
the RCU string series. Patches 6 and 7 are minor cleanups found by
sparse. Patches 3-7 can wait for 4.14 or 4.15.

Based on 4.13-rc6.

Thanks!

1: https://lwn.net/Articles/629048/

Omar Sandoval (7):
  Btrfs: fix blk_status_t/errno confusion
  Btrfs: fix incorrect {node,sector}size endianness from
    BTRFS_IOC_FS_INFO
  Move Btrfs RCU string to common library
  Btrfs: refactor btrfs_device->name updates
  Btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO
  Btrfs: make some volumes.c functions static
  Btrfs: fix __user casting in ioctl.c

 fs/btrfs/check-integrity.c |  12 +--
 fs/btrfs/dev-replace.c     |  37 +++++----
 fs/btrfs/disk-io.c         |  10 +--
 fs/btrfs/extent_io.c       |   8 +-
 fs/btrfs/inode.c           |  70 ++++++++--------
 fs/btrfs/ioctl.c           |  30 +++----
 fs/btrfs/raid56.c          |  35 ++++----
 fs/btrfs/rcu-string.h      |  56 -------------
 fs/btrfs/scrub.c           |  42 +++++-----
 fs/btrfs/super.c           |   7 +-
 fs/btrfs/volumes.c         | 199 +++++++++++++++++++++++++++------------------
 fs/btrfs/volumes.h         |   8 +-
 include/linux/rcustring.h  |  97 ++++++++++++++++++++++
 13 files changed, 351 insertions(+), 260 deletions(-)
 delete mode 100644 fs/btrfs/rcu-string.h
 create mode 100644 include/linux/rcustring.h

-- 
2.14.1


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

* [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion
  2017-08-23  6:45 [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings Omar Sandoval
@ 2017-08-23  6:45 ` Omar Sandoval
  2017-08-23 15:50   ` David Sterba
  2017-08-23 16:13   ` Liu Bo
  2017-08-23  6:46 ` [PATCH 2/7] Btrfs: fix incorrect {node,sector}size endianness from BTRFS_IOC_FS_INFO Omar Sandoval
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23  6:45 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

From: Omar Sandoval <osandov@fb.com>

This fixes several instances of blk_status_t and bare errno ints being
mixed up, some of which are real bugs.

Fixes: 4e4cbee93d56 ("block: switch bios to blk_status_t")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/disk-io.c |  4 ++--
 fs/btrfs/inode.c   | 70 +++++++++++++++++++++++++++++-------------------------
 fs/btrfs/raid56.c  | 34 +++++++++++++-------------
 fs/btrfs/volumes.c | 10 ++++----
 fs/btrfs/volumes.h |  6 ++---
 5 files changed, 64 insertions(+), 60 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 080e2ebb8aa0..f45b61fe9a9a 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3516,7 +3516,7 @@ static blk_status_t wait_dev_flush(struct btrfs_device *device)
 	struct bio *bio = device->flush_bio;
 
 	if (!device->flush_bio_sent)
-		return 0;
+		return BLK_STS_OK;
 
 	device->flush_bio_sent = 0;
 	wait_for_completion_io(&device->flush_wait);
@@ -3563,7 +3563,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
 			continue;
 
 		write_dev_flush(dev);
-		dev->last_flush_error = 0;
+		dev->last_flush_error = BLK_STS_OK;
 	}
 
 	/* wait for all the barriers */
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 95c212037095..24bcd5cd9cf2 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -7924,11 +7924,12 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
 	return ret;
 }
 
-static inline int submit_dio_repair_bio(struct inode *inode, struct bio *bio,
-					int mirror_num)
+static inline blk_status_t submit_dio_repair_bio(struct inode *inode,
+						 struct bio *bio,
+						 int mirror_num)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	int ret;
+	blk_status_t ret;
 
 	BUG_ON(bio_op(bio) == REQ_OP_WRITE);
 
@@ -7980,10 +7981,10 @@ static int btrfs_check_dio_repairable(struct inode *inode,
 	return 1;
 }
 
-static int dio_read_error(struct inode *inode, struct bio *failed_bio,
-			struct page *page, unsigned int pgoff,
-			u64 start, u64 end, int failed_mirror,
-			bio_end_io_t *repair_endio, void *repair_arg)
+static blk_status_t dio_read_error(struct inode *inode, struct bio *failed_bio,
+				   struct page *page, unsigned int pgoff,
+				   u64 start, u64 end, int failed_mirror,
+				   bio_end_io_t *repair_endio, void *repair_arg)
 {
 	struct io_failure_record *failrec;
 	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
@@ -7993,18 +7994,19 @@ static int dio_read_error(struct inode *inode, struct bio *failed_bio,
 	int read_mode = 0;
 	int segs;
 	int ret;
+	blk_status_t status;
 
 	BUG_ON(bio_op(failed_bio) == REQ_OP_WRITE);
 
 	ret = btrfs_get_io_failure_record(inode, start, end, &failrec);
 	if (ret)
-		return ret;
+		return errno_to_blk_status(ret);
 
 	ret = btrfs_check_dio_repairable(inode, failed_bio, failrec,
 					 failed_mirror);
 	if (!ret) {
 		free_io_failure(failure_tree, io_tree, failrec);
-		return -EIO;
+		return BLK_STS_IOERR;
 	}
 
 	segs = bio_segments(failed_bio);
@@ -8022,13 +8024,13 @@ static int dio_read_error(struct inode *inode, struct bio *failed_bio,
 		    "Repair DIO Read Error: submitting new dio read[%#x] to this_mirror=%d, in_validation=%d\n",
 		    read_mode, failrec->this_mirror, failrec->in_validation);
 
-	ret = submit_dio_repair_bio(inode, bio, failrec->this_mirror);
-	if (ret) {
+	status = submit_dio_repair_bio(inode, bio, failrec->this_mirror);
+	if (status) {
 		free_io_failure(failure_tree, io_tree, failrec);
 		bio_put(bio);
 	}
 
-	return ret;
+	return status;
 }
 
 struct btrfs_retry_complete {
@@ -8065,8 +8067,8 @@ static void btrfs_retry_endio_nocsum(struct bio *bio)
 	bio_put(bio);
 }
 
-static int __btrfs_correct_data_nocsum(struct inode *inode,
-				       struct btrfs_io_bio *io_bio)
+static blk_status_t __btrfs_correct_data_nocsum(struct inode *inode,
+						struct btrfs_io_bio *io_bio)
 {
 	struct btrfs_fs_info *fs_info;
 	struct bio_vec bvec;
@@ -8076,8 +8078,8 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
 	unsigned int pgoff;
 	u32 sectorsize;
 	int nr_sectors;
-	int ret;
-	int err = 0;
+	blk_status_t ret;
+	blk_status_t err = BLK_STS_OK;
 
 	fs_info = BTRFS_I(inode)->root->fs_info;
 	sectorsize = fs_info->sectorsize;
@@ -8183,11 +8185,12 @@ static blk_status_t __btrfs_subio_endio_read(struct inode *inode,
 	int csum_pos;
 	bool uptodate = (err == 0);
 	int ret;
+	blk_status_t status;
 
 	fs_info = BTRFS_I(inode)->root->fs_info;
 	sectorsize = fs_info->sectorsize;
 
-	err = 0;
+	err = BLK_STS_OK;
 	start = io_bio->logical;
 	done.inode = inode;
 	io_bio->bio.bi_iter = io_bio->iter;
@@ -8209,12 +8212,12 @@ static blk_status_t __btrfs_subio_endio_read(struct inode *inode,
 		done.start = start;
 		init_completion(&done.done);
 
-		ret = dio_read_error(inode, &io_bio->bio, bvec.bv_page,
-				pgoff, start, start + sectorsize - 1,
-				io_bio->mirror_num,
-				btrfs_retry_endio, &done);
-		if (ret) {
-			err = errno_to_blk_status(ret);
+		status = dio_read_error(inode, &io_bio->bio, bvec.bv_page,
+					pgoff, start, start + sectorsize - 1,
+					io_bio->mirror_num, btrfs_retry_endio,
+					&done);
+		if (status) {
+			err = status;
 			goto next;
 		}
 
@@ -8250,7 +8253,7 @@ static blk_status_t btrfs_subio_endio_read(struct inode *inode,
 		if (unlikely(err))
 			return __btrfs_correct_data_nocsum(inode, io_bio);
 		else
-			return 0;
+			return BLK_STS_OK;
 	} else {
 		return __btrfs_subio_endio_read(inode, io_bio, err);
 	}
@@ -8423,9 +8426,9 @@ static inline blk_status_t btrfs_lookup_and_bind_dio_csum(struct inode *inode,
 	return 0;
 }
 
-static inline int __btrfs_submit_dio_bio(struct bio *bio, struct inode *inode,
-					 u64 file_offset, int skip_sum,
-					 int async_submit)
+static inline blk_status_t
+__btrfs_submit_dio_bio(struct bio *bio, struct inode *inode, u64 file_offset,
+		       int skip_sum, int async_submit)
 {
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
 	struct btrfs_dio_private *dip = bio->bi_private;
@@ -8488,6 +8491,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
 	int clone_offset = 0;
 	int clone_len;
 	int ret;
+	blk_status_t status;
 
 	map_length = orig_bio->bi_iter.bi_size;
 	submit_len = map_length;
@@ -8537,9 +8541,9 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
 		 */
 		atomic_inc(&dip->pending_bios);
 
-		ret = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
-					     async_submit);
-		if (ret) {
+		status = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
+						async_submit);
+		if (status) {
 			bio_put(bio);
 			atomic_dec(&dip->pending_bios);
 			goto out_err;
@@ -8557,9 +8561,9 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
 	} while (submit_len > 0);
 
 submit:
-	ret = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
-				     async_submit);
-	if (!ret)
+	status = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
+					async_submit);
+	if (!status)
 		return 0;
 
 	bio_put(bio);
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 208638384cd2..2cf6ba40f7c4 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -905,7 +905,7 @@ static void raid_write_end_io(struct bio *bio)
 	if (!atomic_dec_and_test(&rbio->stripes_pending))
 		return;
 
-	err = 0;
+	err = BLK_STS_OK;
 
 	/* OK, we have read all the stripes we need to. */
 	max_errors = (rbio->operation == BTRFS_RBIO_PARITY_SCRUB) ?
@@ -1324,7 +1324,7 @@ static noinline void finish_rmw(struct btrfs_raid_bio *rbio)
 	return;
 
 cleanup:
-	rbio_orig_end_io(rbio, -EIO);
+	rbio_orig_end_io(rbio, BLK_STS_IOERR);
 }
 
 /*
@@ -1475,7 +1475,7 @@ static void raid_rmw_end_io(struct bio *bio)
 
 cleanup:
 
-	rbio_orig_end_io(rbio, -EIO);
+	rbio_orig_end_io(rbio, BLK_STS_IOERR);
 }
 
 static void async_rmw_stripe(struct btrfs_raid_bio *rbio)
@@ -1579,7 +1579,7 @@ static int raid56_rmw_stripe(struct btrfs_raid_bio *rbio)
 	return 0;
 
 cleanup:
-	rbio_orig_end_io(rbio, -EIO);
+	rbio_orig_end_io(rbio, BLK_STS_IOERR);
 	return -EIO;
 
 finish:
@@ -1795,12 +1795,12 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 	void **pointers;
 	int faila = -1, failb = -1;
 	struct page *page;
-	int err;
+	blk_status_t err;
 	int i;
 
 	pointers = kcalloc(rbio->real_stripes, sizeof(void *), GFP_NOFS);
 	if (!pointers) {
-		err = -ENOMEM;
+		err = BLK_STS_RESOURCE;
 		goto cleanup_io;
 	}
 
@@ -1856,7 +1856,7 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 					 * a bad data or Q stripe.
 					 * TODO, we should redo the xor here.
 					 */
-					err = -EIO;
+					err = BLK_STS_IOERR;
 					goto cleanup;
 				}
 				/*
@@ -1882,7 +1882,7 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 			if (rbio->bbio->raid_map[failb] == RAID6_Q_STRIPE) {
 				if (rbio->bbio->raid_map[faila] ==
 				    RAID5_P_STRIPE) {
-					err = -EIO;
+					err = BLK_STS_IOERR;
 					goto cleanup;
 				}
 				/*
@@ -1954,13 +1954,13 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 		}
 	}
 
-	err = 0;
+	err = BLK_STS_OK;
 cleanup:
 	kfree(pointers);
 
 cleanup_io:
 	if (rbio->operation == BTRFS_RBIO_READ_REBUILD) {
-		if (err == 0)
+		if (err == BLK_STS_OK)
 			cache_rbio_pages(rbio);
 		else
 			clear_bit(RBIO_CACHE_READY_BIT, &rbio->flags);
@@ -1968,7 +1968,7 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
 		rbio_orig_end_io(rbio, err);
 	} else if (rbio->operation == BTRFS_RBIO_REBUILD_MISSING) {
 		rbio_orig_end_io(rbio, err);
-	} else if (err == 0) {
+	} else if (err == BLK_STS_OK) {
 		rbio->faila = -1;
 		rbio->failb = -1;
 
@@ -2005,7 +2005,7 @@ static void raid_recover_end_io(struct bio *bio)
 		return;
 
 	if (atomic_read(&rbio->error) > rbio->bbio->max_errors)
-		rbio_orig_end_io(rbio, -EIO);
+		rbio_orig_end_io(rbio, BLK_STS_IOERR);
 	else
 		__raid_recover_end_io(rbio);
 }
@@ -2104,7 +2104,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
 cleanup:
 	if (rbio->operation == BTRFS_RBIO_READ_REBUILD ||
 	    rbio->operation == BTRFS_RBIO_REBUILD_MISSING)
-		rbio_orig_end_io(rbio, -EIO);
+		rbio_orig_end_io(rbio, BLK_STS_IOERR);
 	return -EIO;
 }
 
@@ -2431,7 +2431,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 	nr_data = bio_list_size(&bio_list);
 	if (!nr_data) {
 		/* Every parity is right */
-		rbio_orig_end_io(rbio, 0);
+		rbio_orig_end_io(rbio, BLK_STS_OK);
 		return;
 	}
 
@@ -2451,7 +2451,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
 	return;
 
 cleanup:
-	rbio_orig_end_io(rbio, -EIO);
+	rbio_orig_end_io(rbio, BLK_STS_IOERR);
 }
 
 static inline int is_data_stripe(struct btrfs_raid_bio *rbio, int stripe)
@@ -2519,7 +2519,7 @@ static void validate_rbio_for_parity_scrub(struct btrfs_raid_bio *rbio)
 	return;
 
 cleanup:
-	rbio_orig_end_io(rbio, -EIO);
+	rbio_orig_end_io(rbio, BLK_STS_IOERR);
 }
 
 /*
@@ -2633,7 +2633,7 @@ static void raid56_parity_scrub_stripe(struct btrfs_raid_bio *rbio)
 	return;
 
 cleanup:
-	rbio_orig_end_io(rbio, -EIO);
+	rbio_orig_end_io(rbio, BLK_STS_IOERR);
 	return;
 
 finish:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e8b9a269fdde..bd679bc7a1a9 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6212,8 +6212,8 @@ static void bbio_error(struct btrfs_bio *bbio, struct bio *bio, u64 logical)
 	}
 }
 
-int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
-		  int mirror_num, int async_submit)
+blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
+			   int mirror_num, int async_submit)
 {
 	struct btrfs_device *dev;
 	struct bio *first_bio = bio;
@@ -6233,7 +6233,7 @@ int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 				&map_length, &bbio, mirror_num, 1);
 	if (ret) {
 		btrfs_bio_counter_dec(fs_info);
-		return ret;
+		return errno_to_blk_status(ret);
 	}
 
 	total_devs = bbio->num_stripes;
@@ -6256,7 +6256,7 @@ int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 		}
 
 		btrfs_bio_counter_dec(fs_info);
-		return ret;
+		return errno_to_blk_status(ret);
 	}
 
 	if (map_length < length) {
@@ -6283,7 +6283,7 @@ int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
 				  dev_nr, async_submit);
 	}
 	btrfs_bio_counter_dec(fs_info);
-	return 0;
+	return BLK_STS_OK;
 }
 
 struct btrfs_device *btrfs_find_device(struct btrfs_fs_info *fs_info, u64 devid,
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6f45fd60d15a..93277fc60930 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -74,7 +74,7 @@ struct btrfs_device {
 	int missing;
 	int can_discard;
 	int is_tgtdev_for_dev_replace;
-	int last_flush_error;
+	blk_status_t last_flush_error;
 	int flush_bio_sent;
 
 #ifdef __BTRFS_NEED_DEVICE_DATA_ORDERED
@@ -416,8 +416,8 @@ int btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 		      struct btrfs_fs_info *fs_info, u64 type);
 void btrfs_mapping_init(struct btrfs_mapping_tree *tree);
 void btrfs_mapping_tree_free(struct btrfs_mapping_tree *tree);
-int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
-		  int mirror_num, int async_submit);
+blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
+			   int mirror_num, int async_submit);
 int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       fmode_t flags, void *holder);
 int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
-- 
2.14.1


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

* [PATCH 2/7] Btrfs: fix incorrect {node,sector}size endianness from BTRFS_IOC_FS_INFO
  2017-08-23  6:45 [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings Omar Sandoval
  2017-08-23  6:45 ` [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion Omar Sandoval
@ 2017-08-23  6:46 ` Omar Sandoval
  2017-09-06 14:51   ` David Sterba
  2017-08-23  6:46 ` [PATCH 3/7] Move Btrfs RCU string to common library Omar Sandoval
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23  6:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

From: Omar Sandoval <osandov@fb.com>

fs_info->super_copy->{node,sector}size are little-endian, but the ioctl
should return the values in native endianness. Use the cached values in
btrfs_fs_info instead. Found with sparse.

Fixes: 80a773fbfc2d ("btrfs: retrieve more info from FS_INFO ioctl")
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/ioctl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index fa1b78cf25f6..b51124d5b8a3 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2804,9 +2804,9 @@ static long btrfs_ioctl_fs_info(struct btrfs_fs_info *fs_info,
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
 
-	fi_args->nodesize = fs_info->super_copy->nodesize;
-	fi_args->sectorsize = fs_info->super_copy->sectorsize;
-	fi_args->clone_alignment = fs_info->super_copy->sectorsize;
+	fi_args->nodesize = fs_info->nodesize;
+	fi_args->sectorsize = fs_info->sectorsize;
+	fi_args->clone_alignment = fs_info->sectorsize;
 
 	if (copy_to_user(arg, fi_args, sizeof(*fi_args)))
 		ret = -EFAULT;
-- 
2.14.1


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

* [PATCH 3/7] Move Btrfs RCU string to common library
  2017-08-23  6:45 [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings Omar Sandoval
  2017-08-23  6:45 ` [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion Omar Sandoval
  2017-08-23  6:46 ` [PATCH 2/7] Btrfs: fix incorrect {node,sector}size endianness from BTRFS_IOC_FS_INFO Omar Sandoval
@ 2017-08-23  6:46 ` Omar Sandoval
  2017-09-06 15:15   ` David Sterba
  2017-08-23  6:46 ` [PATCH 4/7] Btrfs: refactor btrfs_device->name updates Omar Sandoval
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23  6:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

From: Omar Sandoval <osandov@fb.com>

The RCU-friendly string API used internally by Btrfs is generic enough
for common use. This doesn't add any new functionality but instead just
moves the code and documents the existing API.

Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/check-integrity.c | 12 +++---
 fs/btrfs/dev-replace.c     | 37 +++++++++---------
 fs/btrfs/disk-io.c         |  6 +--
 fs/btrfs/extent_io.c       |  8 ++--
 fs/btrfs/ioctl.c           |  4 +-
 fs/btrfs/raid56.c          |  1 -
 fs/btrfs/rcu-string.h      | 56 --------------------------
 fs/btrfs/scrub.c           | 42 ++++++++++----------
 fs/btrfs/super.c           |  7 ++--
 fs/btrfs/volumes.c         | 58 ++++++++++++++-------------
 include/linux/rcustring.h  | 97 ++++++++++++++++++++++++++++++++++++++++++++++
 11 files changed, 187 insertions(+), 141 deletions(-)
 delete mode 100644 fs/btrfs/rcu-string.h
 create mode 100644 include/linux/rcustring.h

diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
index 11d37c94ce05..4a09a2d2fcf8 100644
--- a/fs/btrfs/check-integrity.c
+++ b/fs/btrfs/check-integrity.c
@@ -95,6 +95,7 @@
 #include <linux/genhd.h>
 #include <linux/blkdev.h>
 #include <linux/mm.h>
+#include <linux/rcustring.h>
 #include <linux/string.h>
 #include "ctree.h"
 #include "disk-io.h"
@@ -105,7 +106,6 @@
 #include "print-tree.h"
 #include "locking.h"
 #include "check-integrity.h"
-#include "rcu-string.h"
 #include "compression.h"
 
 #define BTRFSIC_BLOCK_HASHTABLE_SIZE 0x10000
@@ -834,11 +834,11 @@ static int btrfsic_process_superblock_dev_mirror(
 		superblock_tmp->mirror_num = 1 + superblock_mirror_num;
 		if (state->print_mask & BTRFSIC_PRINT_MASK_SUPERBLOCK_WRITE)
 			btrfs_info_in_rcu(fs_info,
-				"new initial S-block (bdev %p, %s) @%llu (%s/%llu/%d)",
-				     superblock_bdev,
-				     rcu_str_deref(device->name), dev_bytenr,
-				     dev_state->name, dev_bytenr,
-				     superblock_mirror_num);
+					  "new initial S-block (bdev %p, %s) @%llu (%s/%llu/%d)",
+					  superblock_bdev,
+					  rcu_string_dereference(device->name),
+					  dev_bytenr, dev_state->name,
+					  dev_bytenr, superblock_mirror_num);
 		list_add(&superblock_tmp->all_blocks_node,
 			 &state->all_blocks_list);
 		btrfsic_block_hashtable_add(superblock_tmp,
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index bee3edeea7a3..bd5ba61e4071 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -25,6 +25,7 @@
 #include <linux/capability.h>
 #include <linux/kthread.h>
 #include <linux/math64.h>
+#include <linux/rcustring.h>
 #include <asm/div64.h>
 #include "ctree.h"
 #include "extent_map.h"
@@ -34,7 +35,6 @@
 #include "volumes.h"
 #include "async-thread.h"
 #include "check-integrity.h"
-#include "rcu-string.h"
 #include "dev-replace.h"
 #include "sysfs.h"
 
@@ -362,11 +362,11 @@ int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 	dev_replace->tgtdev = tgt_device;
 
 	btrfs_info_in_rcu(fs_info,
-		      "dev_replace from %s (devid %llu) to %s started",
-		      src_device->missing ? "<missing disk>" :
-		        rcu_str_deref(src_device->name),
-		      src_device->devid,
-		      rcu_str_deref(tgt_device->name));
+			  "dev_replace from %s (devid %llu) to %s started",
+			  src_device->missing ? "<missing disk>" :
+			  rcu_string_dereference(src_device->name),
+			  src_device->devid,
+			  rcu_string_dereference(tgt_device->name));
 
 	/*
 	 * from now on, the writes to the srcdev are all duplicated to
@@ -539,9 +539,10 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 		btrfs_err_in_rcu(fs_info,
 				 "btrfs_scrub_dev(%s, %llu, %s) failed %d",
 				 src_device->missing ? "<missing disk>" :
-				 rcu_str_deref(src_device->name),
+				 rcu_string_dereference(src_device->name),
 				 src_device->devid,
-				 rcu_str_deref(tgt_device->name), scrub_ret);
+				 rcu_string_dereference(tgt_device->name),
+				 scrub_ret);
 		btrfs_dev_replace_unlock(dev_replace, 1);
 		mutex_unlock(&fs_info->chunk_mutex);
 		mutex_unlock(&fs_info->fs_devices->device_list_mutex);
@@ -558,9 +559,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 	btrfs_info_in_rcu(fs_info,
 			  "dev_replace from %s (devid %llu) to %s finished",
 			  src_device->missing ? "<missing disk>" :
-			  rcu_str_deref(src_device->name),
+			  rcu_string_dereference(src_device->name),
 			  src_device->devid,
-			  rcu_str_deref(tgt_device->name));
+			  rcu_string_dereference(tgt_device->name));
 	tgt_device->is_tgtdev_for_dev_replace = 0;
 	tgt_device->devid = src_device->devid;
 	src_device->devid = BTRFS_DEV_REPLACE_DEVID;
@@ -805,14 +806,14 @@ static int btrfs_dev_replace_kthread(void *data)
 		kfree(status_args);
 		progress = div_u64(progress, 10);
 		btrfs_info_in_rcu(fs_info,
-			"continuing dev_replace from %s (devid %llu) to %s @%u%%",
-			dev_replace->srcdev->missing ? "<missing disk>" :
-			rcu_str_deref(dev_replace->srcdev->name),
-			dev_replace->srcdev->devid,
-			dev_replace->tgtdev ?
-			rcu_str_deref(dev_replace->tgtdev->name) :
-			"<missing target disk>",
-			(unsigned int)progress);
+				  "continuing dev_replace from %s (devid %llu) to %s @%u%%",
+				  dev_replace->srcdev->missing ? "<missing disk>" :
+				  rcu_string_dereference(dev_replace->srcdev->name),
+				  dev_replace->srcdev->devid,
+				  dev_replace->tgtdev ?
+				  rcu_string_dereference(dev_replace->tgtdev->name) :
+				  "<missing target disk>",
+				  (unsigned int)progress);
 	}
 	btrfs_dev_replace_continue_on_mount(fs_info);
 	clear_bit(BTRFS_FS_EXCL_OP, &fs_info->flags);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f45b61fe9a9a..e0d08c904506 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/migrate.h>
 #include <linux/ratelimit.h>
+#include <linux/rcustring.h>
 #include <linux/uuid.h>
 #include <linux/semaphore.h>
 #include <asm/unaligned.h>
@@ -44,7 +45,6 @@
 #include "free-space-tree.h"
 #include "inode-map.h"
 #include "check-integrity.h"
-#include "rcu-string.h"
 #include "dev-replace.h"
 #include "raid56.h"
 #include "sysfs.h"
@@ -3298,8 +3298,8 @@ static void btrfs_end_buffer_write_sync(struct buffer_head *bh, int uptodate)
 			bh->b_private;
 
 		btrfs_warn_rl_in_rcu(device->fs_info,
-				"lost page write due to IO error on %s",
-					  rcu_str_deref(device->name));
+				     "lost page write due to IO error on %s",
+				     rcu_string_dereference(device->name));
 		/* note, we don't set_buffer_write_io_error because we have
 		 * our own ways of dealing with the IO errors
 		 */
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0aff9b278c19..4eff5a7d6551 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -6,6 +6,7 @@
 #include <linux/page-flags.h>
 #include <linux/spinlock.h>
 #include <linux/blkdev.h>
+#include <linux/rcustring.h>
 #include <linux/swap.h>
 #include <linux/writeback.h>
 #include <linux/pagevec.h>
@@ -18,7 +19,6 @@
 #include "volumes.h"
 #include "check-integrity.h"
 #include "locking.h"
-#include "rcu-string.h"
 #include "backref.h"
 #include "transaction.h"
 
@@ -2046,9 +2046,9 @@ int repair_io_failure(struct btrfs_fs_info *fs_info, u64 ino, u64 start,
 	}
 
 	btrfs_info_rl_in_rcu(fs_info,
-		"read error corrected: ino %llu off %llu (dev %s sector %llu)",
-				  ino, start,
-				  rcu_str_deref(dev->name), sector);
+			     "read error corrected: ino %llu off %llu (dev %s sector %llu)",
+			     ino, start, rcu_string_dereference(dev->name),
+			     sector);
 	btrfs_bio_counter_dec(fs_info);
 	bio_put(bio);
 	return 0;
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b51124d5b8a3..befeacd0e847 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -31,6 +31,7 @@
 #include <linux/mount.h>
 #include <linux/mpage.h>
 #include <linux/namei.h>
+#include <linux/rcustring.h>
 #include <linux/swap.h>
 #include <linux/writeback.h>
 #include <linux/compat.h>
@@ -52,7 +53,6 @@
 #include "locking.h"
 #include "inode-map.h"
 #include "backref.h"
-#include "rcu-string.h"
 #include "send.h"
 #include "dev-replace.h"
 #include "props.h"
@@ -1604,7 +1604,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
 	new_size *= fs_info->sectorsize;
 
 	btrfs_info_in_rcu(fs_info, "new size for %s is %llu",
-			  rcu_str_deref(device->name), new_size);
+			  rcu_string_dereference(device->name), new_size);
 
 	if (new_size > old_size) {
 		trans = btrfs_start_transaction(root, 0);
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 2cf6ba40f7c4..56fc03e7b7b5 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -42,7 +42,6 @@
 #include "raid56.h"
 #include "async-thread.h"
 #include "check-integrity.h"
-#include "rcu-string.h"
 
 /* set when additional merges to this rbio are not allowed */
 #define RBIO_RMW_LOCKED_BIT	1
diff --git a/fs/btrfs/rcu-string.h b/fs/btrfs/rcu-string.h
deleted file mode 100644
index 9e111e4576d4..000000000000
--- a/fs/btrfs/rcu-string.h
+++ /dev/null
@@ -1,56 +0,0 @@
-/*
- * Copyright (C) 2012 Red Hat.  All rights reserved.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public
- * License v2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public
- * License along with this program; if not, write to the
- * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
- * Boston, MA 021110-1307, USA.
- */
-
-struct rcu_string {
-	struct rcu_head rcu;
-	char str[0];
-};
-
-static inline struct rcu_string *rcu_string_strdup(const char *src, gfp_t mask)
-{
-	size_t len = strlen(src) + 1;
-	struct rcu_string *ret = kzalloc(sizeof(struct rcu_string) +
-					 (len * sizeof(char)), mask);
-	if (!ret)
-		return ret;
-	strncpy(ret->str, src, len);
-	return ret;
-}
-
-static inline void rcu_string_free(struct rcu_string *str)
-{
-	if (str)
-		kfree_rcu(str, rcu);
-}
-
-#define printk_in_rcu(fmt, ...) do {	\
-	rcu_read_lock();		\
-	printk(fmt, __VA_ARGS__);	\
-	rcu_read_unlock();		\
-} while (0)
-
-#define printk_ratelimited_in_rcu(fmt, ...) do {	\
-	rcu_read_lock();				\
-	printk_ratelimited(fmt, __VA_ARGS__);		\
-	rcu_read_unlock();				\
-} while (0)
-
-#define rcu_str_deref(rcu_str) ({				\
-	struct rcu_string *__str = rcu_dereference(rcu_str);	\
-	__str->str;						\
-})
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 6f1e4c984b94..d5976c1b1714 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -18,6 +18,7 @@
 
 #include <linux/blkdev.h>
 #include <linux/ratelimit.h>
+#include <linux/rcustring.h>
 #include <linux/sched/mm.h>
 #include "ctree.h"
 #include "volumes.h"
@@ -28,7 +29,6 @@
 #include "extent_io.h"
 #include "dev-replace.h"
 #include "check-integrity.h"
-#include "rcu-string.h"
 #include "raid56.h"
 
 /*
@@ -799,7 +799,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 		btrfs_warn_in_rcu(fs_info,
 				  "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)",
 				  swarn->errstr, swarn->logical,
-				  rcu_str_deref(swarn->dev->name),
+				  rcu_string_dereference(swarn->dev->name),
 				  (unsigned long long)swarn->sector,
 				  root, inum, offset,
 				  min(isize - offset, (u64)PAGE_SIZE), nlink,
@@ -812,7 +812,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root,
 	btrfs_warn_in_rcu(fs_info,
 			  "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu: path resolving failed with ret=%d",
 			  swarn->errstr, swarn->logical,
-			  rcu_str_deref(swarn->dev->name),
+			  rcu_string_dereference(swarn->dev->name),
 			  (unsigned long long)swarn->sector,
 			  root, inum, offset, ret);
 
@@ -868,13 +868,13 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock)
 						      item_size, &ref_root,
 						      &ref_level);
 			btrfs_warn_in_rcu(fs_info,
-				"%s at logical %llu on dev %s, sector %llu: metadata %s (level %d) in tree %llu",
-				errstr, swarn.logical,
-				rcu_str_deref(dev->name),
-				(unsigned long long)swarn.sector,
-				ref_level ? "node" : "leaf",
-				ret < 0 ? -1 : ref_level,
-				ret < 0 ? -1 : ref_root);
+					  "%s at logical %llu on dev %s, sector %llu: metadata %s (level %d) in tree %llu",
+					  errstr, swarn.logical,
+					  rcu_string_dereference(dev->name),
+					  (unsigned long long)swarn.sector,
+					  ref_level ? "node" : "leaf",
+					  ret < 0 ? -1 : ref_level,
+					  ret < 0 ? -1 : ref_root);
 		} while (ret != 1);
 		btrfs_release_path(path);
 	} else {
@@ -1068,8 +1068,9 @@ static void scrub_fixup_nodatasum(struct btrfs_work *work)
 		btrfs_dev_replace_stats_inc(
 			&fs_info->dev_replace.num_uncorrectable_read_errors);
 		btrfs_err_rl_in_rcu(fs_info,
-		    "unable to fixup (nodatasum) error at logical %llu on dev %s",
-			fixup->logical, rcu_str_deref(fixup->dev->name));
+				    "unable to fixup (nodatasum) error at logical %llu on dev %s",
+				    fixup->logical,
+				    rcu_string_dereference(fixup->dev->name));
 	}
 
 	btrfs_free_path(path);
@@ -1458,8 +1459,9 @@ static int scrub_handle_errored_block(struct scrub_block *sblock_to_check)
 			sblock_to_check->data_corrected = 1;
 			spin_unlock(&sctx->stat_lock);
 			btrfs_err_rl_in_rcu(fs_info,
-				"fixed up error at logical %llu on dev %s",
-				logical, rcu_str_deref(dev->name));
+					    "fixed up error at logical %llu on dev %s",
+					    logical,
+					    rcu_string_dereference(dev->name));
 		}
 	} else {
 did_not_correct_error:
@@ -1467,8 +1469,8 @@ static int scrub_handle_errored_block(struct scrub_block *sblock_to_check)
 		sctx->stat.uncorrectable_errors++;
 		spin_unlock(&sctx->stat_lock);
 		btrfs_err_rl_in_rcu(fs_info,
-			"unable to fixup (regular) error at logical %llu on dev %s",
-			logical, rcu_str_deref(dev->name));
+				    "unable to fixup (regular) error at logical %llu on dev %s",
+				    logical, rcu_string_dereference(dev->name));
 	}
 
 out:
@@ -2387,15 +2389,15 @@ static void scrub_missing_raid56_worker(struct btrfs_work *work)
 		sctx->stat.read_errors++;
 		spin_unlock(&sctx->stat_lock);
 		btrfs_err_rl_in_rcu(fs_info,
-			"IO error rebuilding logical %llu for dev %s",
-			logical, rcu_str_deref(dev->name));
+				    "IO error rebuilding logical %llu for dev %s",
+				    logical, rcu_string_dereference(dev->name));
 	} else if (sblock->header_error || sblock->checksum_error) {
 		spin_lock(&sctx->stat_lock);
 		sctx->stat.uncorrectable_errors++;
 		spin_unlock(&sctx->stat_lock);
 		btrfs_err_rl_in_rcu(fs_info,
-			"failed to rebuild valid logical %llu for dev %s",
-			logical, rcu_str_deref(dev->name));
+				    "failed to rebuild valid logical %llu for dev %s",
+				    logical, rcu_string_dereference(dev->name));
 	} else {
 		scrub_write_block_to_dev_replace(sblock);
 	}
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 12540b6104b5..e051fa3bfd93 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -42,6 +42,7 @@
 #include <linux/cleancache.h>
 #include <linux/ratelimit.h>
 #include <linux/btrfs.h>
+#include <linux/rcustring.h>
 #include "delayed-inode.h"
 #include "ctree.h"
 #include "disk-io.h"
@@ -54,7 +55,6 @@
 #include "volumes.h"
 #include "export.h"
 #include "compression.h"
-#include "rcu-string.h"
 #include "dev-replace.h"
 #include "free-space-cache.h"
 #include "backref.h"
@@ -2221,7 +2221,6 @@ static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
 	struct btrfs_fs_devices *cur_devices;
 	struct btrfs_device *dev, *first_dev = NULL;
 	struct list_head *head;
-	struct rcu_string *name;
 
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	cur_devices = fs_info->fs_devices;
@@ -2240,8 +2239,8 @@ static int btrfs_show_devname(struct seq_file *m, struct dentry *root)
 
 	if (first_dev) {
 		rcu_read_lock();
-		name = rcu_dereference(first_dev->name);
-		seq_escape(m, name->str, " \t\n\\");
+		seq_escape(m, rcu_string_dereference(first_dev->name),
+			   " \t\n\\");
 		rcu_read_unlock();
 	} else {
 		WARN_ON(1);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bd679bc7a1a9..4010c35898d8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -25,6 +25,7 @@
 #include <linux/ratelimit.h>
 #include <linux/kthread.h>
 #include <linux/raid/pq.h>
+#include <linux/rcustring.h>
 #include <linux/semaphore.h>
 #include <linux/uuid.h>
 #include <asm/div64.h>
@@ -37,7 +38,6 @@
 #include "raid56.h"
 #include "async-thread.h"
 #include "check-integrity.h"
-#include "rcu-string.h"
 #include "math.h"
 #include "dev-replace.h"
 #include "sysfs.h"
@@ -584,8 +584,8 @@ void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 			 * either use mapper or non mapper path throughout.
 			 */
 			rcu_read_lock();
-			del = strcmp(rcu_str_deref(dev->name),
-						rcu_str_deref(cur_dev->name));
+			del = strcmp(rcu_string_dereference(dev->name),
+				     rcu_string_dereference(cur_dev->name));
 			rcu_read_unlock();
 			if (!del)
 				break;
@@ -3540,8 +3540,9 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 			ret = PTR_ERR(trans);
 			btrfs_info_in_rcu(fs_info,
 		 "resize: unable to start transaction after shrinking device %s (error %d), old size %llu, new size %llu",
-					  rcu_str_deref(device->name), ret,
-					  old_size, old_size - size_to_free);
+					  rcu_string_dereference(device->name),
+					  ret, old_size,
+					  old_size - size_to_free);
 			goto error;
 		}
 
@@ -3552,8 +3553,9 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 			WARN_ON(ret > 0);
 			btrfs_info_in_rcu(fs_info,
 		 "resize: unable to grow device after shrinking device %s (error %d), old size %llu, new size %llu",
-					  rcu_str_deref(device->name), ret,
-					  old_size, old_size - size_to_free);
+					  rcu_string_dereference(device->name),
+					  ret, old_size,
+					  old_size - size_to_free);
 			goto error;
 		}
 
@@ -7014,8 +7016,8 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
 	ret = btrfs_search_slot(trans, dev_root, &key, path, -1, 1);
 	if (ret < 0) {
 		btrfs_warn_in_rcu(fs_info,
-			"error %d while searching for dev_stats item for device %s",
-			      ret, rcu_str_deref(device->name));
+				  "error %d while searching for dev_stats item for device %s",
+				  ret, rcu_string_dereference(device->name));
 		goto out;
 	}
 
@@ -7025,8 +7027,9 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
 		ret = btrfs_del_item(trans, dev_root, path);
 		if (ret != 0) {
 			btrfs_warn_in_rcu(fs_info,
-				"delete too small dev_stats item for device %s failed %d",
-				      rcu_str_deref(device->name), ret);
+					  "delete too small dev_stats item for device %s failed %d",
+					  rcu_string_dereference(device->name),
+					  ret);
 			goto out;
 		}
 		ret = 1;
@@ -7039,8 +7042,9 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
 					      &key, sizeof(*ptr));
 		if (ret < 0) {
 			btrfs_warn_in_rcu(fs_info,
-				"insert dev_stats item for device %s failed %d",
-				rcu_str_deref(device->name), ret);
+					  "insert dev_stats item for device %s failed %d",
+					  rcu_string_dereference(device->name),
+					  ret);
 			goto out;
 		}
 	}
@@ -7094,13 +7098,13 @@ static void btrfs_dev_stat_print_on_error(struct btrfs_device *dev)
 	if (!dev->dev_stats_valid)
 		return;
 	btrfs_err_rl_in_rcu(dev->fs_info,
-		"bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
-			   rcu_str_deref(dev->name),
-			   btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
-			   btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
-			   btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_FLUSH_ERRS),
-			   btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_CORRUPTION_ERRS),
-			   btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_GENERATION_ERRS));
+			    "bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
+			    rcu_string_dereference(dev->name),
+			    btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
+			    btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
+			    btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_FLUSH_ERRS),
+			    btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_CORRUPTION_ERRS),
+			    btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_GENERATION_ERRS));
 }
 
 static void btrfs_dev_stat_print_on_load(struct btrfs_device *dev)
@@ -7114,13 +7118,13 @@ static void btrfs_dev_stat_print_on_load(struct btrfs_device *dev)
 		return; /* all values == 0, suppress message */
 
 	btrfs_info_in_rcu(dev->fs_info,
-		"bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
-	       rcu_str_deref(dev->name),
-	       btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
-	       btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
-	       btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_FLUSH_ERRS),
-	       btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_CORRUPTION_ERRS),
-	       btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_GENERATION_ERRS));
+			  "bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u",
+			  rcu_string_dereference(dev->name),
+			  btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS),
+			  btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_READ_ERRS),
+			  btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_FLUSH_ERRS),
+			  btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_CORRUPTION_ERRS),
+			  btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_GENERATION_ERRS));
 }
 
 int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info,
diff --git a/include/linux/rcustring.h b/include/linux/rcustring.h
new file mode 100644
index 000000000000..d73cfcd7de5a
--- /dev/null
+++ b/include/linux/rcustring.h
@@ -0,0 +1,97 @@
+/*
+ * RCU-friendly strings
+ *
+ * Copyright (C) 2012 Red Hat.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, you can access it online at
+ * http://www.gnu.org/licenses/gpl-2.0.html.
+ */
+
+#ifndef _LINUX_RCUSTRING_H
+#define _LINUX_RCUSTRING_H
+
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <linux/printk.h>
+#include <linux/rcupdate.h>
+#include <linux/slab.h>
+
+struct rcu_string {
+	struct rcu_head rcu;
+	char str[0];
+};
+
+/**
+ * rcu_string_strdup() - create an RCU string from a string
+ * @src: The string to copy
+ * @flags: Flags for kmalloc
+ */
+static inline struct rcu_string *rcu_string_strdup(const char *src, gfp_t flags)
+{
+	struct rcu_string *ret;
+	size_t len = strlen(src) + 1;
+
+	ret = kmalloc(sizeof(*ret) + (len * sizeof(char)), flags);
+	if (ret)
+		memcpy(ret->str, src, len);
+	return ret;
+}
+
+/**
+ * rcu_string_free() - free an RCU string
+ * @str: The string
+ */
+static inline void rcu_string_free(struct rcu_string *str)
+{
+	if (str)
+		kfree_rcu(str, rcu);
+}
+
+/**
+ * rcu_string_dereference() - dereference an RCU string
+ * @str: The string
+ *
+ * Like rcu_dereference, this must be done in an RCU critical section.
+ */
+static inline char *rcu_string_dereference(struct rcu_string __rcu *rcu_str)
+{
+	return rcu_dereference(rcu_str)->str;
+}
+
+/**
+ * printk_in_rcu() - printk in an RCU read-side critical section
+ * @fmt: Format string
+ * @...: Values
+ */
+#define printk_in_rcu(fmt, ...)			\
+	do {					\
+		rcu_read_lock();		\
+		printk(fmt, ##__VA_ARGS__);	\
+		rcu_read_unlock();		\
+	} while (0)
+
+/**
+ * printk_ratelimited_in_rcu() - printk_ratelimited in an RCU read-side critical
+ * section
+ * @fmt: Format string
+ * @...: Values
+ */
+#define printk_ratelimited_in_rcu(fmt, ...)		\
+	do {						\
+		rcu_read_lock();			\
+		printk_ratelimited(fmt, ##__VA_ARGS__);	\
+		rcu_read_unlock();			\
+	} while (0)
+
+#endif
-- 
2.14.1


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

* [PATCH 4/7] Btrfs: refactor btrfs_device->name updates
  2017-08-23  6:45 [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings Omar Sandoval
                   ` (2 preceding siblings ...)
  2017-08-23  6:46 ` [PATCH 3/7] Move Btrfs RCU string to common library Omar Sandoval
@ 2017-08-23  6:46 ` Omar Sandoval
  2017-08-23  6:46 ` [PATCH 5/7] Btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO Omar Sandoval
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23  6:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

From: Omar Sandoval <osandov@fb.com>

The rcu_string API introduced some new sparse errors but also revealed
existing ones. First of all, the name in struct btrfs_device should be
annotated as __rcu to prevent unsafe reads. Additionally, updates should
go through rcu_dereference_protected to make it clear what's going on.
This introduces some helper functions that factor out this
functionality.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/volumes.c | 117 +++++++++++++++++++++++++++++++++++------------------
 fs/btrfs/volumes.h |   2 +-
 2 files changed, 79 insertions(+), 40 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4010c35898d8..9b7a8e6257ee 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -152,6 +152,46 @@ struct list_head *btrfs_get_fs_uuids(void)
 	return &fs_uuids;
 }
 
+/*
+ * Dereference the device name under the uuid_mutex.
+ */
+static inline struct rcu_string *
+btrfs_dev_rcu_protected_name(struct btrfs_device *dev)
+	__must_hold(&uuid_mutex)
+{
+	return rcu_dereference_protected(dev->name,
+					 lockdep_is_held(&uuid_mutex));
+}
+
+/*
+ * Use when the caller is the only possible updater.
+ */
+static inline struct rcu_string *
+btrfs_dev_rcu_only_name(struct btrfs_device *dev)
+{
+	return rcu_dereference_protected(dev->name, 1);
+}
+
+/*
+ * Rename a device under the uuid_mutex.
+ */
+static inline int btrfs_dev_rename(struct btrfs_device *dev, const char *name,
+				   gfp_t gfp_mask)
+	__must_hold(&uuid_mutex)
+{
+	struct rcu_string *old_name, *new_name;
+
+	new_name = rcu_string_strdup(name, gfp_mask);
+	if (!new_name)
+		return -ENOMEM;
+
+	old_name = btrfs_dev_rcu_protected_name(dev);
+	rcu_assign_pointer(dev->name, new_name);
+	rcu_string_free(old_name);
+
+	return 0;
+}
+
 static struct btrfs_fs_devices *__alloc_fs_devices(void)
 {
 	struct btrfs_fs_devices *fs_devs;
@@ -203,7 +243,7 @@ static void free_fs_devices(struct btrfs_fs_devices *fs_devices)
 		device = list_entry(fs_devices->devices.next,
 				    struct btrfs_device, dev_list);
 		list_del(&device->dev_list);
-		rcu_string_free(device->name);
+		rcu_string_free(btrfs_dev_rcu_only_name(device));
 		kfree(device);
 	}
 	kfree(fs_devices);
@@ -600,7 +640,7 @@ void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 			} else {
 				fs_devs->num_devices--;
 				list_del(&dev->dev_list);
-				rcu_string_free(dev->name);
+				rcu_string_free(btrfs_dev_rcu_only_name(dev));
 				kfree(dev);
 			}
 			break;
@@ -651,12 +691,10 @@ static noinline int device_list_add(const char *path,
 			return PTR_ERR(device);
 		}
 
-		name = rcu_string_strdup(path, GFP_NOFS);
-		if (!name) {
+		if (btrfs_dev_rename(device, path, GFP_NOFS)) {
 			kfree(device);
 			return -ENOMEM;
 		}
-		rcu_assign_pointer(device->name, name);
 
 		mutex_lock(&fs_devices->device_list_mutex);
 		list_add_rcu(&device->dev_list, &fs_devices->devices);
@@ -665,13 +703,17 @@ static noinline int device_list_add(const char *path,
 
 		ret = 1;
 		device->fs_devices = fs_devices;
-	} else if (!device->name || strcmp(device->name->str, path)) {
+	} else {
+		name = btrfs_dev_rcu_protected_name(device);
+		if (name && strcmp(name->str, path) == 0)
+			goto out;
+
 		/*
 		 * When FS is already mounted.
-		 * 1. If you are here and if the device->name is NULL that
-		 *    means this device was missing at time of FS mount.
-		 * 2. If you are here and if the device->name is different
-		 *    from 'path' that means either
+		 * 1. If you are here and if the name is NULL that means this
+		 *    device was missing at time of FS mount.
+		 * 2. If you are here and if the name is different from 'path'
+		 *    that means either
 		 *      a. The same device disappeared and reappeared with
 		 *         different name. or
 		 *      b. The missing-disk-which-was-replaced, has
@@ -703,17 +745,15 @@ static noinline int device_list_add(const char *path,
 			return -EEXIST;
 		}
 
-		name = rcu_string_strdup(path, GFP_NOFS);
-		if (!name)
+		if (btrfs_dev_rename(device, path, GFP_NOFS))
 			return -ENOMEM;
-		rcu_string_free(device->name);
-		rcu_assign_pointer(device->name, name);
 		if (device->missing) {
 			fs_devices->missing_devices--;
 			device->missing = 0;
 		}
 	}
 
+out:
 	/*
 	 * Unmount does not free the btrfs_device struct but would zero
 	 * generation along with most of the other members. So just update
@@ -757,18 +797,12 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig)
 		if (IS_ERR(device))
 			goto error;
 
-		/*
-		 * This is ok to do without rcu read locked because we hold the
-		 * uuid mutex so nothing we touch in here is going to disappear.
-		 */
-		if (orig_dev->name) {
-			name = rcu_string_strdup(orig_dev->name->str,
-					GFP_KERNEL);
-			if (!name) {
+		name = btrfs_dev_rcu_protected_name(orig_dev);
+		if (name) {
+			if (btrfs_dev_rename(device, name->str, GFP_KERNEL)) {
 				kfree(device);
 				goto error;
 			}
-			rcu_assign_pointer(device->name, name);
 		}
 
 		list_add(&device->dev_list, &fs_devices->devices);
@@ -829,7 +863,7 @@ void btrfs_close_extra_devices(struct btrfs_fs_devices *fs_devices, int step)
 		}
 		list_del_init(&device->dev_list);
 		fs_devices->num_devices--;
-		rcu_string_free(device->name);
+		rcu_string_free(btrfs_dev_rcu_only_name(device));
 		kfree(device);
 	}
 
@@ -848,7 +882,7 @@ static void __free_device(struct work_struct *work)
 	struct btrfs_device *device;
 
 	device = container_of(work, struct btrfs_device, rcu_work);
-	rcu_string_free(device->name);
+	rcu_string_free(btrfs_dev_rcu_only_name(device));
 	bio_put(device->flush_bio);
 	kfree(device);
 }
@@ -896,11 +930,10 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device)
 					device->uuid);
 	BUG_ON(IS_ERR(new_device)); /* -ENOMEM */
 
-	/* Safe because we are under uuid_mutex */
-	if (device->name) {
-		name = rcu_string_strdup(device->name->str, GFP_NOFS);
-		BUG_ON(!name); /* -ENOMEM */
-		rcu_assign_pointer(new_device->name, name);
+	name = btrfs_dev_rcu_protected_name(device);
+	if (name) {
+		if (btrfs_dev_rename(new_device, name->str, GFP_NOFS))
+			BUG_ON(1); /* -ENOMEM */
 	}
 
 	list_replace_rcu(&device->dev_list, &new_device->dev_list);
@@ -987,18 +1020,20 @@ static int __btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 	u64 devid;
 	int seeding = 1;
 	int ret = 0;
+	struct rcu_string *name;
 
 	flags |= FMODE_EXCL;
 
 	list_for_each_entry(device, head, dev_list) {
 		if (device->bdev)
 			continue;
-		if (!device->name)
+		name = btrfs_dev_rcu_protected_name(device);
+		if (!name)
 			continue;
 
 		/* Just open everything we can; ignore failures here */
-		if (btrfs_get_bdev_and_sb(device->name->str, flags, holder, 1,
-					    &bdev, &bh))
+		if (btrfs_get_bdev_and_sb(name->str, flags, holder, 1, &bdev,
+					  &bh))
 			continue;
 
 		disk_super = (struct btrfs_super_block *)bh->b_data;
@@ -1966,8 +2001,10 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	 * the devices list.  All that's left is to zero out the old
 	 * supers and free the device.
 	 */
-	if (device->writeable)
-		btrfs_scratch_superblocks(device->bdev, device->name->str);
+	if (device->writeable) {
+		btrfs_scratch_superblocks(device->bdev,
+					  btrfs_dev_rcu_protected_name(device)->str);
+	}
 
 	btrfs_close_bdev(device);
 	call_rcu(&device->rcu, free_device);
@@ -2040,7 +2077,8 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_fs_info *fs_info,
 
 	if (srcdev->writeable) {
 		/* zero out the old super if it is writable */
-		btrfs_scratch_superblocks(srcdev->bdev, srcdev->name->str);
+		btrfs_scratch_superblocks(srcdev->bdev,
+					  btrfs_dev_rcu_only_name(srcdev)->str);
 	}
 
 	btrfs_close_bdev(srcdev);
@@ -2099,7 +2137,8 @@ void btrfs_destroy_dev_replace_tgtdev(struct btrfs_fs_info *fs_info,
 	 * is already out of device list, so we don't have to hold
 	 * the device_list_mutex lock.
 	 */
-	btrfs_scratch_superblocks(tgtdev->bdev, tgtdev->name->str);
+	btrfs_scratch_superblocks(tgtdev->bdev,
+				  btrfs_dev_rcu_only_name(tgtdev)->str);
 
 	btrfs_close_bdev(tgtdev);
 	call_rcu(&tgtdev->rcu, free_device);
@@ -2383,7 +2422,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
-		rcu_string_free(device->name);
+		rcu_string_free(btrfs_dev_rcu_only_name(device));
 		kfree(device);
 		ret = PTR_ERR(trans);
 		goto error;
@@ -2517,7 +2556,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 
 error_trans:
 	btrfs_end_transaction(trans);
-	rcu_string_free(device->name);
+	rcu_string_free(btrfs_dev_rcu_only_name(device));
 	btrfs_sysfs_rm_device_link(fs_info->fs_devices, device);
 	kfree(device);
 error:
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 93277fc60930..f3b1c77a4fee 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -53,7 +53,7 @@ struct btrfs_device {
 	struct btrfs_fs_devices *fs_devices;
 	struct btrfs_fs_info *fs_info;
 
-	struct rcu_string *name;
+	struct rcu_string __rcu *name;
 
 	u64 generation;
 
-- 
2.14.1


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

* [PATCH 5/7] Btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO
  2017-08-23  6:45 [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings Omar Sandoval
                   ` (3 preceding siblings ...)
  2017-08-23  6:46 ` [PATCH 4/7] Btrfs: refactor btrfs_device->name updates Omar Sandoval
@ 2017-08-23  6:46 ` Omar Sandoval
  2017-09-06 15:37   ` David Sterba
  2017-08-23  6:46 ` [PATCH 6/7] Btrfs: make some volumes.c functions static Omar Sandoval
  2017-08-23  6:46 ` [PATCH 7/7] Btrfs: fix __user casting in ioctl.c Omar Sandoval
  6 siblings, 1 reply; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23  6:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

From: Omar Sandoval <osandov@fb.com>

A naked read of the value of an RCU pointer isn't safe. Put the whole
access in an RCU critical section, not just the pointer dereference.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/ioctl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index befeacd0e847..cf71d0304671 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2823,6 +2823,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	int ret = 0;
 	char *s_uuid = NULL;
+	struct rcu_string *name;
 
 	di_args = memdup_user(arg, sizeof(*di_args));
 	if (IS_ERR(di_args))
@@ -2843,17 +2844,16 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info,
 	di_args->bytes_used = btrfs_device_get_bytes_used(dev);
 	di_args->total_bytes = btrfs_device_get_total_bytes(dev);
 	memcpy(di_args->uuid, dev->uuid, sizeof(di_args->uuid));
-	if (dev->name) {
-		struct rcu_string *name;
 
-		rcu_read_lock();
-		name = rcu_dereference(dev->name);
+	rcu_read_lock();
+	name = rcu_dereference(dev->name);
+	if (name) {
 		strncpy(di_args->path, name->str, sizeof(di_args->path));
-		rcu_read_unlock();
 		di_args->path[sizeof(di_args->path) - 1] = 0;
 	} else {
 		di_args->path[0] = '\0';
 	}
+	rcu_read_unlock();
 
 out:
 	mutex_unlock(&fs_devices->device_list_mutex);
-- 
2.14.1


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

* [PATCH 6/7] Btrfs: make some volumes.c functions static
  2017-08-23  6:45 [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings Omar Sandoval
                   ` (4 preceding siblings ...)
  2017-08-23  6:46 ` [PATCH 5/7] Btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO Omar Sandoval
@ 2017-08-23  6:46 ` Omar Sandoval
  2017-09-06 15:25   ` David Sterba
  2017-08-23  6:46 ` [PATCH 7/7] Btrfs: fix __user casting in ioctl.c Omar Sandoval
  6 siblings, 1 reply; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23  6:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

From: Omar Sandoval <osandov@fb.com>

These aren't used outside of volumes.c.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/volumes.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9b7a8e6257ee..9f2ed74a2c81 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -594,7 +594,7 @@ static void pending_bios_fn(struct btrfs_work *work)
 }
 
 
-void btrfs_free_stale_device(struct btrfs_device *cur_dev)
+static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
 {
 	struct btrfs_fs_devices *fs_devs;
 	struct btrfs_device *dev;
@@ -1110,14 +1110,15 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 	return ret;
 }
 
-void btrfs_release_disk_super(struct page *page)
+static void btrfs_release_disk_super(struct page *page)
 {
 	kunmap(page);
 	put_page(page);
 }
 
-int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
-		struct page **page, struct btrfs_super_block **disk_super)
+static int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
+				 struct page **page,
+				 struct btrfs_super_block **disk_super)
 {
 	void *p;
 	pgoff_t index;
@@ -1860,8 +1861,9 @@ static int btrfs_check_raid_min_devices(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-struct btrfs_device *btrfs_find_next_active_device(struct btrfs_fs_devices *fs_devs,
-					struct btrfs_device *device)
+static struct btrfs_device *
+btrfs_find_next_active_device(struct btrfs_fs_devices *fs_devs,
+			      struct btrfs_device *device)
 {
 	struct btrfs_device *next_device;
 
-- 
2.14.1


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

* [PATCH 7/7] Btrfs: fix __user casting in ioctl.c
  2017-08-23  6:45 [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings Omar Sandoval
                   ` (5 preceding siblings ...)
  2017-08-23  6:46 ` [PATCH 6/7] Btrfs: make some volumes.c functions static Omar Sandoval
@ 2017-08-23  6:46 ` Omar Sandoval
  2017-09-06 15:23   ` David Sterba
  6 siblings, 1 reply; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23  6:46 UTC (permalink / raw)
  To: linux-btrfs; +Cc: kernel-team

From: Omar Sandoval <osandov@fb.com>

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/ioctl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index cf71d0304671..7a7a82e0963c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2210,7 +2210,7 @@ static noinline int btrfs_ioctl_tree_search_v2(struct file *file,
 
 	inode = file_inode(file);
 	ret = search_ioctl(inode, &args.key, &buf_size,
-			   (char *)(&uarg->buf[0]));
+			   (char __user *)(&uarg->buf[0]));
 	if (ret == 0 && copy_to_user(&uarg->key, &args.key, sizeof(args.key)))
 		ret = -EFAULT;
 	else if (ret == -EOVERFLOW &&
@@ -4512,8 +4512,8 @@ static long btrfs_ioctl_ino_to_path(struct btrfs_root *root, void __user *arg)
 		ipath->fspath->val[i] = rel_ptr;
 	}
 
-	ret = copy_to_user((void *)(unsigned long)ipa->fspath,
-			   (void *)(unsigned long)ipath->fspath, size);
+	ret = copy_to_user((void __user *)(unsigned long)ipa->fspath,
+			   ipath->fspath, size);
 	if (ret) {
 		ret = -EFAULT;
 		goto out;
@@ -4584,8 +4584,8 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_fs_info *fs_info,
 	if (ret < 0)
 		goto out;
 
-	ret = copy_to_user((void *)(unsigned long)loi->inodes,
-			   (void *)(unsigned long)inodes, size);
+	ret = copy_to_user((void __user *)(unsigned long)loi->inodes, inodes,
+			   size);
 	if (ret)
 		ret = -EFAULT;
 
-- 
2.14.1


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

* Re: [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion
  2017-08-23  6:45 ` [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion Omar Sandoval
@ 2017-08-23 15:50   ` David Sterba
  2017-08-23 16:13   ` Liu Bo
  1 sibling, 0 replies; 18+ messages in thread
From: David Sterba @ 2017-08-23 15:50 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-btrfs, kernel-team, clm

On Tue, Aug 22, 2017 at 11:45:59PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This fixes several instances of blk_status_t and bare errno ints being
> mixed up, some of which are real bugs.

Reviewed-by: David Sterba <dsterba@suse.com>

The real bugs are lurking in the error cases, other than BLK_STS_OK that
is equivalent to the old return values.

Seems like blk_status_t conversion of raid56.c has been entirely
skipped, and maybe some of the changes brought by the btrfs pull. Thanks
for catching it. I'll prepare a pull request with this one for 4.13-rc7.
The other patch 2/7 is not that urgent and can wait.

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

* Re: [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion
  2017-08-23  6:45 ` [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion Omar Sandoval
  2017-08-23 15:50   ` David Sterba
@ 2017-08-23 16:13   ` Liu Bo
  2017-08-23 19:18     ` Omar Sandoval
  1 sibling, 1 reply; 18+ messages in thread
From: Liu Bo @ 2017-08-23 16:13 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-btrfs, kernel-team

On Tue, Aug 22, 2017 at 11:45:59PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This fixes several instances of blk_status_t and bare errno ints being
> mixed up, some of which are real bugs.
> 
> Fixes: 4e4cbee93d56 ("block: switch bios to blk_status_t")
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  fs/btrfs/disk-io.c |  4 ++--
>  fs/btrfs/inode.c   | 70 +++++++++++++++++++++++++++++-------------------------
>  fs/btrfs/raid56.c  | 34 +++++++++++++-------------
>  fs/btrfs/volumes.c | 10 ++++----
>  fs/btrfs/volumes.h |  6 ++---
>  5 files changed, 64 insertions(+), 60 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 080e2ebb8aa0..f45b61fe9a9a 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3516,7 +3516,7 @@ static blk_status_t wait_dev_flush(struct btrfs_device *device)
>  	struct bio *bio = device->flush_bio;
>  
>  	if (!device->flush_bio_sent)
> -		return 0;
> +		return BLK_STS_OK;
>  
>  	device->flush_bio_sent = 0;
>  	wait_for_completion_io(&device->flush_wait);
> @@ -3563,7 +3563,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
>  			continue;
>  
>  		write_dev_flush(dev);
> -		dev->last_flush_error = 0;
> +		dev->last_flush_error = BLK_STS_OK;
>  	}
>  
>  	/* wait for all the barriers */
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 95c212037095..24bcd5cd9cf2 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7924,11 +7924,12 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
>  	return ret;
>  }
>  
> -static inline int submit_dio_repair_bio(struct inode *inode, struct bio *bio,
> -					int mirror_num)
> +static inline blk_status_t submit_dio_repair_bio(struct inode *inode,
> +						 struct bio *bio,
> +						 int mirror_num)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> -	int ret;
> +	blk_status_t ret;
>  
>  	BUG_ON(bio_op(bio) == REQ_OP_WRITE);
>

In this function, there is btrfs_map_bio() who returns 0 or error
code, errno_to_blk_status() is also needed for that conversion.

With that,

Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks,

-liubo

> @@ -7980,10 +7981,10 @@ static int btrfs_check_dio_repairable(struct inode *inode,
>  	return 1;
>  }
>  
> -static int dio_read_error(struct inode *inode, struct bio *failed_bio,
> -			struct page *page, unsigned int pgoff,
> -			u64 start, u64 end, int failed_mirror,
> -			bio_end_io_t *repair_endio, void *repair_arg)
> +static blk_status_t dio_read_error(struct inode *inode, struct bio *failed_bio,
> +				   struct page *page, unsigned int pgoff,
> +				   u64 start, u64 end, int failed_mirror,
> +				   bio_end_io_t *repair_endio, void *repair_arg)
>  {
>  	struct io_failure_record *failrec;
>  	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> @@ -7993,18 +7994,19 @@ static int dio_read_error(struct inode *inode, struct bio *failed_bio,
>  	int read_mode = 0;
>  	int segs;
>  	int ret;
> +	blk_status_t status;
>  
>  	BUG_ON(bio_op(failed_bio) == REQ_OP_WRITE);
>  
>  	ret = btrfs_get_io_failure_record(inode, start, end, &failrec);
>  	if (ret)
> -		return ret;
> +		return errno_to_blk_status(ret);
>  
>  	ret = btrfs_check_dio_repairable(inode, failed_bio, failrec,
>  					 failed_mirror);
>  	if (!ret) {
>  		free_io_failure(failure_tree, io_tree, failrec);
> -		return -EIO;
> +		return BLK_STS_IOERR;
>  	}
>  
>  	segs = bio_segments(failed_bio);
> @@ -8022,13 +8024,13 @@ static int dio_read_error(struct inode *inode, struct bio *failed_bio,
>  		    "Repair DIO Read Error: submitting new dio read[%#x] to this_mirror=%d, in_validation=%d\n",
>  		    read_mode, failrec->this_mirror, failrec->in_validation);
>  
> -	ret = submit_dio_repair_bio(inode, bio, failrec->this_mirror);
> -	if (ret) {
> +	status = submit_dio_repair_bio(inode, bio, failrec->this_mirror);
> +	if (status) {
>  		free_io_failure(failure_tree, io_tree, failrec);
>  		bio_put(bio);
>  	}
>  
> -	return ret;
> +	return status;
>  }
>  
>  struct btrfs_retry_complete {
> @@ -8065,8 +8067,8 @@ static void btrfs_retry_endio_nocsum(struct bio *bio)
>  	bio_put(bio);
>  }
>  
> -static int __btrfs_correct_data_nocsum(struct inode *inode,
> -				       struct btrfs_io_bio *io_bio)
> +static blk_status_t __btrfs_correct_data_nocsum(struct inode *inode,
> +						struct btrfs_io_bio *io_bio)
>  {
>  	struct btrfs_fs_info *fs_info;
>  	struct bio_vec bvec;
> @@ -8076,8 +8078,8 @@ static int __btrfs_correct_data_nocsum(struct inode *inode,
>  	unsigned int pgoff;
>  	u32 sectorsize;
>  	int nr_sectors;
> -	int ret;
> -	int err = 0;
> +	blk_status_t ret;
> +	blk_status_t err = BLK_STS_OK;
>  
>  	fs_info = BTRFS_I(inode)->root->fs_info;
>  	sectorsize = fs_info->sectorsize;
> @@ -8183,11 +8185,12 @@ static blk_status_t __btrfs_subio_endio_read(struct inode *inode,
>  	int csum_pos;
>  	bool uptodate = (err == 0);
>  	int ret;
> +	blk_status_t status;
>  
>  	fs_info = BTRFS_I(inode)->root->fs_info;
>  	sectorsize = fs_info->sectorsize;
>  
> -	err = 0;
> +	err = BLK_STS_OK;
>  	start = io_bio->logical;
>  	done.inode = inode;
>  	io_bio->bio.bi_iter = io_bio->iter;
> @@ -8209,12 +8212,12 @@ static blk_status_t __btrfs_subio_endio_read(struct inode *inode,
>  		done.start = start;
>  		init_completion(&done.done);
>  
> -		ret = dio_read_error(inode, &io_bio->bio, bvec.bv_page,
> -				pgoff, start, start + sectorsize - 1,
> -				io_bio->mirror_num,
> -				btrfs_retry_endio, &done);
> -		if (ret) {
> -			err = errno_to_blk_status(ret);
> +		status = dio_read_error(inode, &io_bio->bio, bvec.bv_page,
> +					pgoff, start, start + sectorsize - 1,
> +					io_bio->mirror_num, btrfs_retry_endio,
> +					&done);
> +		if (status) {
> +			err = status;
>  			goto next;
>  		}
>  
> @@ -8250,7 +8253,7 @@ static blk_status_t btrfs_subio_endio_read(struct inode *inode,
>  		if (unlikely(err))
>  			return __btrfs_correct_data_nocsum(inode, io_bio);
>  		else
> -			return 0;
> +			return BLK_STS_OK;
>  	} else {
>  		return __btrfs_subio_endio_read(inode, io_bio, err);
>  	}
> @@ -8423,9 +8426,9 @@ static inline blk_status_t btrfs_lookup_and_bind_dio_csum(struct inode *inode,
>  	return 0;
>  }
>  
> -static inline int __btrfs_submit_dio_bio(struct bio *bio, struct inode *inode,
> -					 u64 file_offset, int skip_sum,
> -					 int async_submit)
> +static inline blk_status_t
> +__btrfs_submit_dio_bio(struct bio *bio, struct inode *inode, u64 file_offset,
> +		       int skip_sum, int async_submit)
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct btrfs_dio_private *dip = bio->bi_private;
> @@ -8488,6 +8491,7 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
>  	int clone_offset = 0;
>  	int clone_len;
>  	int ret;
> +	blk_status_t status;
>  
>  	map_length = orig_bio->bi_iter.bi_size;
>  	submit_len = map_length;
> @@ -8537,9 +8541,9 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
>  		 */
>  		atomic_inc(&dip->pending_bios);
>  
> -		ret = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
> -					     async_submit);
> -		if (ret) {
> +		status = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
> +						async_submit);
> +		if (status) {
>  			bio_put(bio);
>  			atomic_dec(&dip->pending_bios);
>  			goto out_err;
> @@ -8557,9 +8561,9 @@ static int btrfs_submit_direct_hook(struct btrfs_dio_private *dip,
>  	} while (submit_len > 0);
>  
>  submit:
> -	ret = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
> -				     async_submit);
> -	if (!ret)
> +	status = __btrfs_submit_dio_bio(bio, inode, file_offset, skip_sum,
> +					async_submit);
> +	if (!status)
>  		return 0;
>  
>  	bio_put(bio);
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 208638384cd2..2cf6ba40f7c4 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -905,7 +905,7 @@ static void raid_write_end_io(struct bio *bio)
>  	if (!atomic_dec_and_test(&rbio->stripes_pending))
>  		return;
>  
> -	err = 0;
> +	err = BLK_STS_OK;
>  
>  	/* OK, we have read all the stripes we need to. */
>  	max_errors = (rbio->operation == BTRFS_RBIO_PARITY_SCRUB) ?
> @@ -1324,7 +1324,7 @@ static noinline void finish_rmw(struct btrfs_raid_bio *rbio)
>  	return;
>  
>  cleanup:
> -	rbio_orig_end_io(rbio, -EIO);
> +	rbio_orig_end_io(rbio, BLK_STS_IOERR);
>  }
>  
>  /*
> @@ -1475,7 +1475,7 @@ static void raid_rmw_end_io(struct bio *bio)
>  
>  cleanup:
>  
> -	rbio_orig_end_io(rbio, -EIO);
> +	rbio_orig_end_io(rbio, BLK_STS_IOERR);
>  }
>  
>  static void async_rmw_stripe(struct btrfs_raid_bio *rbio)
> @@ -1579,7 +1579,7 @@ static int raid56_rmw_stripe(struct btrfs_raid_bio *rbio)
>  	return 0;
>  
>  cleanup:
> -	rbio_orig_end_io(rbio, -EIO);
> +	rbio_orig_end_io(rbio, BLK_STS_IOERR);
>  	return -EIO;
>  
>  finish:
> @@ -1795,12 +1795,12 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
>  	void **pointers;
>  	int faila = -1, failb = -1;
>  	struct page *page;
> -	int err;
> +	blk_status_t err;
>  	int i;
>  
>  	pointers = kcalloc(rbio->real_stripes, sizeof(void *), GFP_NOFS);
>  	if (!pointers) {
> -		err = -ENOMEM;
> +		err = BLK_STS_RESOURCE;
>  		goto cleanup_io;
>  	}
>  
> @@ -1856,7 +1856,7 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
>  					 * a bad data or Q stripe.
>  					 * TODO, we should redo the xor here.
>  					 */
> -					err = -EIO;
> +					err = BLK_STS_IOERR;
>  					goto cleanup;
>  				}
>  				/*
> @@ -1882,7 +1882,7 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
>  			if (rbio->bbio->raid_map[failb] == RAID6_Q_STRIPE) {
>  				if (rbio->bbio->raid_map[faila] ==
>  				    RAID5_P_STRIPE) {
> -					err = -EIO;
> +					err = BLK_STS_IOERR;
>  					goto cleanup;
>  				}
>  				/*
> @@ -1954,13 +1954,13 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
>  		}
>  	}
>  
> -	err = 0;
> +	err = BLK_STS_OK;
>  cleanup:
>  	kfree(pointers);
>  
>  cleanup_io:
>  	if (rbio->operation == BTRFS_RBIO_READ_REBUILD) {
> -		if (err == 0)
> +		if (err == BLK_STS_OK)
>  			cache_rbio_pages(rbio);
>  		else
>  			clear_bit(RBIO_CACHE_READY_BIT, &rbio->flags);
> @@ -1968,7 +1968,7 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
>  		rbio_orig_end_io(rbio, err);
>  	} else if (rbio->operation == BTRFS_RBIO_REBUILD_MISSING) {
>  		rbio_orig_end_io(rbio, err);
> -	} else if (err == 0) {
> +	} else if (err == BLK_STS_OK) {
>  		rbio->faila = -1;
>  		rbio->failb = -1;
>  
> @@ -2005,7 +2005,7 @@ static void raid_recover_end_io(struct bio *bio)
>  		return;
>  
>  	if (atomic_read(&rbio->error) > rbio->bbio->max_errors)
> -		rbio_orig_end_io(rbio, -EIO);
> +		rbio_orig_end_io(rbio, BLK_STS_IOERR);
>  	else
>  		__raid_recover_end_io(rbio);
>  }
> @@ -2104,7 +2104,7 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
>  cleanup:
>  	if (rbio->operation == BTRFS_RBIO_READ_REBUILD ||
>  	    rbio->operation == BTRFS_RBIO_REBUILD_MISSING)
> -		rbio_orig_end_io(rbio, -EIO);
> +		rbio_orig_end_io(rbio, BLK_STS_IOERR);
>  	return -EIO;
>  }
>  
> @@ -2431,7 +2431,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
>  	nr_data = bio_list_size(&bio_list);
>  	if (!nr_data) {
>  		/* Every parity is right */
> -		rbio_orig_end_io(rbio, 0);
> +		rbio_orig_end_io(rbio, BLK_STS_OK);
>  		return;
>  	}
>  
> @@ -2451,7 +2451,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
>  	return;
>  
>  cleanup:
> -	rbio_orig_end_io(rbio, -EIO);
> +	rbio_orig_end_io(rbio, BLK_STS_IOERR);
>  }
>  
>  static inline int is_data_stripe(struct btrfs_raid_bio *rbio, int stripe)
> @@ -2519,7 +2519,7 @@ static void validate_rbio_for_parity_scrub(struct btrfs_raid_bio *rbio)
>  	return;
>  
>  cleanup:
> -	rbio_orig_end_io(rbio, -EIO);
> +	rbio_orig_end_io(rbio, BLK_STS_IOERR);
>  }
>  
>  /*
> @@ -2633,7 +2633,7 @@ static void raid56_parity_scrub_stripe(struct btrfs_raid_bio *rbio)
>  	return;
>  
>  cleanup:
> -	rbio_orig_end_io(rbio, -EIO);
> +	rbio_orig_end_io(rbio, BLK_STS_IOERR);
>  	return;
>  
>  finish:
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e8b9a269fdde..bd679bc7a1a9 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6212,8 +6212,8 @@ static void bbio_error(struct btrfs_bio *bbio, struct bio *bio, u64 logical)
>  	}
>  }
>  
> -int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> -		  int mirror_num, int async_submit)
> +blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> +			   int mirror_num, int async_submit)
>  {
>  	struct btrfs_device *dev;
>  	struct bio *first_bio = bio;
> @@ -6233,7 +6233,7 @@ int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>  				&map_length, &bbio, mirror_num, 1);
>  	if (ret) {
>  		btrfs_bio_counter_dec(fs_info);
> -		return ret;
> +		return errno_to_blk_status(ret);
>  	}
>  
>  	total_devs = bbio->num_stripes;
> @@ -6256,7 +6256,7 @@ int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>  		}
>  
>  		btrfs_bio_counter_dec(fs_info);
> -		return ret;
> +		return errno_to_blk_status(ret);
>  	}
>  
>  	if (map_length < length) {
> @@ -6283,7 +6283,7 @@ int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>  				  dev_nr, async_submit);
>  	}
>  	btrfs_bio_counter_dec(fs_info);
> -	return 0;
> +	return BLK_STS_OK;
>  }
>  
>  struct btrfs_device *btrfs_find_device(struct btrfs_fs_info *fs_info, u64 devid,
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 6f45fd60d15a..93277fc60930 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -74,7 +74,7 @@ struct btrfs_device {
>  	int missing;
>  	int can_discard;
>  	int is_tgtdev_for_dev_replace;
> -	int last_flush_error;
> +	blk_status_t last_flush_error;
>  	int flush_bio_sent;
>  
>  #ifdef __BTRFS_NEED_DEVICE_DATA_ORDERED
> @@ -416,8 +416,8 @@ int btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>  		      struct btrfs_fs_info *fs_info, u64 type);
>  void btrfs_mapping_init(struct btrfs_mapping_tree *tree);
>  void btrfs_mapping_tree_free(struct btrfs_mapping_tree *tree);
> -int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> -		  int mirror_num, int async_submit);
> +blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> +			   int mirror_num, int async_submit);
>  int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>  		       fmode_t flags, void *holder);
>  int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
> -- 
> 2.14.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion
  2017-08-23 16:13   ` Liu Bo
@ 2017-08-23 19:18     ` Omar Sandoval
  0 siblings, 0 replies; 18+ messages in thread
From: Omar Sandoval @ 2017-08-23 19:18 UTC (permalink / raw)
  To: Liu Bo; +Cc: linux-btrfs, kernel-team

On Wed, Aug 23, 2017 at 10:13:40AM -0600, Liu Bo wrote:
> On Tue, Aug 22, 2017 at 11:45:59PM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > This fixes several instances of blk_status_t and bare errno ints being
> > mixed up, some of which are real bugs.
> > 
> > Fixes: 4e4cbee93d56 ("block: switch bios to blk_status_t")
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >  fs/btrfs/disk-io.c |  4 ++--
> >  fs/btrfs/inode.c   | 70 +++++++++++++++++++++++++++++-------------------------
> >  fs/btrfs/raid56.c  | 34 +++++++++++++-------------
> >  fs/btrfs/volumes.c | 10 ++++----
> >  fs/btrfs/volumes.h |  6 ++---
> >  5 files changed, 64 insertions(+), 60 deletions(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 080e2ebb8aa0..f45b61fe9a9a 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -3516,7 +3516,7 @@ static blk_status_t wait_dev_flush(struct btrfs_device *device)
> >  	struct bio *bio = device->flush_bio;
> >  
> >  	if (!device->flush_bio_sent)
> > -		return 0;
> > +		return BLK_STS_OK;
> >  
> >  	device->flush_bio_sent = 0;
> >  	wait_for_completion_io(&device->flush_wait);
> > @@ -3563,7 +3563,7 @@ static int barrier_all_devices(struct btrfs_fs_info *info)
> >  			continue;
> >  
> >  		write_dev_flush(dev);
> > -		dev->last_flush_error = 0;
> > +		dev->last_flush_error = BLK_STS_OK;
> >  	}
> >  
> >  	/* wait for all the barriers */
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 95c212037095..24bcd5cd9cf2 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -7924,11 +7924,12 @@ static int btrfs_get_blocks_direct(struct inode *inode, sector_t iblock,
> >  	return ret;
> >  }
> >  
> > -static inline int submit_dio_repair_bio(struct inode *inode, struct bio *bio,
> > -					int mirror_num)
> > +static inline blk_status_t submit_dio_repair_bio(struct inode *inode,
> > +						 struct bio *bio,
> > +						 int mirror_num)
> >  {
> >  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
> > -	int ret;
> > +	blk_status_t ret;
> >  
> >  	BUG_ON(bio_op(bio) == REQ_OP_WRITE);
> >
> 
> In this function, there is btrfs_map_bio() who returns 0 or error
> code, errno_to_blk_status() is also needed for that conversion.

See below, btrfs_map_bio() returns blk_status_t now.

> With that,
> 
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>

Thanks!

> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index e8b9a269fdde..bd679bc7a1a9 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -6212,8 +6212,8 @@ static void bbio_error(struct btrfs_bio *bbio, struct bio *bio, u64 logical)
> >  	}
> >  }
> >  
> > -int btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> > -		  int mirror_num, int async_submit)
> > +blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
> > +			   int mirror_num, int async_submit)

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

* Re: [PATCH 2/7] Btrfs: fix incorrect {node,sector}size endianness from BTRFS_IOC_FS_INFO
  2017-08-23  6:46 ` [PATCH 2/7] Btrfs: fix incorrect {node,sector}size endianness from BTRFS_IOC_FS_INFO Omar Sandoval
@ 2017-09-06 14:51   ` David Sterba
  0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2017-09-06 14:51 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-btrfs, kernel-team

On Tue, Aug 22, 2017 at 11:46:00PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> fs_info->super_copy->{node,sector}size are little-endian, but the ioctl
> should return the values in native endianness. Use the cached values in
> btrfs_fs_info instead. Found with sparse.
> 
> Fixes: 80a773fbfc2d ("btrfs: retrieve more info from FS_INFO ioctl")
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Reviewed-by: David Sterba <dsterba@suse.com>

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

* Re: [PATCH 3/7] Move Btrfs RCU string to common library
  2017-08-23  6:46 ` [PATCH 3/7] Move Btrfs RCU string to common library Omar Sandoval
@ 2017-09-06 15:15   ` David Sterba
  2017-09-07 17:10     ` Omar Sandoval
  0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2017-09-06 15:15 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-btrfs, kernel-team

On Tue, Aug 22, 2017 at 11:46:01PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> The RCU-friendly string API used internally by Btrfs is generic enough
> for common use. This doesn't add any new functionality but instead just
> moves the code and documents the existing API.

Ok for the changes, but please drop anything that just reformats the
argument list and/or shifts the long strings out of 80 columns. I
personally hate the "arguments aligned under the opening (" style and
can somehow live with that in new code, but if it's part of a patch
that's pure noise as I have to read all the changed lines and figure out
if they really are different or not. Thanks.

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

* Re: [PATCH 7/7] Btrfs: fix __user casting in ioctl.c
  2017-08-23  6:46 ` [PATCH 7/7] Btrfs: fix __user casting in ioctl.c Omar Sandoval
@ 2017-09-06 15:23   ` David Sterba
  0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2017-09-06 15:23 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-btrfs, kernel-team

On Tue, Aug 22, 2017 at 11:46:05PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
Reviewed-by: David Sterba <dsterba@suse.com>

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

* Re: [PATCH 6/7] Btrfs: make some volumes.c functions static
  2017-08-23  6:46 ` [PATCH 6/7] Btrfs: make some volumes.c functions static Omar Sandoval
@ 2017-09-06 15:25   ` David Sterba
  0 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2017-09-06 15:25 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-btrfs, kernel-team

On Tue, Aug 22, 2017 at 11:46:04PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> These aren't used outside of volumes.c.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  fs/btrfs/volumes.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 9b7a8e6257ee..9f2ed74a2c81 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -594,7 +594,7 @@ static void pending_bios_fn(struct btrfs_work *work)
>  }
>  
>  
> -void btrfs_free_stale_device(struct btrfs_device *cur_dev)
> +static void btrfs_free_stale_device(struct btrfs_device *cur_dev)
>  {
>  	struct btrfs_fs_devices *fs_devs;
>  	struct btrfs_device *dev;
> @@ -1110,14 +1110,15 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>  	return ret;
>  }
>  
> -void btrfs_release_disk_super(struct page *page)
> +static void btrfs_release_disk_super(struct page *page)
>  {
>  	kunmap(page);
>  	put_page(page);
>  }
>  
> -int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
> -		struct page **page, struct btrfs_super_block **disk_super)
> +static int btrfs_read_disk_super(struct block_device *bdev, u64 bytenr,
> +				 struct page **page,
> +				 struct btrfs_super_block **disk_super)
>  {
>  	void *p;
>  	pgoff_t index;
> @@ -1860,8 +1861,9 @@ static int btrfs_check_raid_min_devices(struct btrfs_fs_info *fs_info,
>  	return 0;
>  }
>  
> -struct btrfs_device *btrfs_find_next_active_device(struct btrfs_fs_devices *fs_devs,
> -					struct btrfs_device *device)
> +static struct btrfs_device *
> +btrfs_find_next_active_device(struct btrfs_fs_devices *fs_devs,
> +			      struct btrfs_device *device)

Please do not switch from the preferred declaration style where the type
is on the same line as the function name. In case it's too long, put
that on the next line.  As this one is trivial, I'll fix that at commit
time, but please keep that in mind in the future.

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

* Re: [PATCH 5/7] Btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO
  2017-08-23  6:46 ` [PATCH 5/7] Btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO Omar Sandoval
@ 2017-09-06 15:37   ` David Sterba
  2017-09-07 17:24     ` Omar Sandoval
  0 siblings, 1 reply; 18+ messages in thread
From: David Sterba @ 2017-09-06 15:37 UTC (permalink / raw)
  To: Omar Sandoval; +Cc: linux-btrfs, kernel-team

On Tue, Aug 22, 2017 at 11:46:03PM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> A naked read of the value of an RCU pointer isn't safe. Put the whole
> access in an RCU critical section, not just the pointer dereference.

In this case it is safe, as the device will not go away (and potentially
free dev->name) because it's under the device_list_mutex.

The locking around devices and related structures is not exactly
straightforward, but here I don't think we need to stick to the RCU
pattern if the protection is guaranteed by other means. This applies to
uuid_mutex and device_list_mutex.

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

* Re: [PATCH 3/7] Move Btrfs RCU string to common library
  2017-09-06 15:15   ` David Sterba
@ 2017-09-07 17:10     ` Omar Sandoval
  0 siblings, 0 replies; 18+ messages in thread
From: Omar Sandoval @ 2017-09-07 17:10 UTC (permalink / raw)
  To: dsterba, linux-btrfs, kernel-team

On Wed, Sep 06, 2017 at 05:15:06PM +0200, David Sterba wrote:
> On Tue, Aug 22, 2017 at 11:46:01PM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > The RCU-friendly string API used internally by Btrfs is generic enough
> > for common use. This doesn't add any new functionality but instead just
> > moves the code and documents the existing API.
> 
> Ok for the changes, but please drop anything that just reformats the
> argument list and/or shifts the long strings out of 80 columns. I
> personally hate the "arguments aligned under the opening (" style

Well that's the kernel style ;)

> and can somehow live with that in new code, but if it's part of a patch
> that's pure noise as I have to read all the changed lines and figure out
> if they really are different or not. Thanks.

I only reflowed calls where I changed something, so that shouldn't be an
issue. If you really prefer I can resend it without reflowing anything,
but that's going to leave the code looking worse than before.

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

* Re: [PATCH 5/7] Btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO
  2017-09-06 15:37   ` David Sterba
@ 2017-09-07 17:24     ` Omar Sandoval
  0 siblings, 0 replies; 18+ messages in thread
From: Omar Sandoval @ 2017-09-07 17:24 UTC (permalink / raw)
  To: dsterba, linux-btrfs, kernel-team

On Wed, Sep 06, 2017 at 05:37:28PM +0200, David Sterba wrote:
> On Tue, Aug 22, 2017 at 11:46:03PM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > A naked read of the value of an RCU pointer isn't safe. Put the whole
> > access in an RCU critical section, not just the pointer dereference.
> 
> In this case it is safe, as the device will not go away (and potentially
> free dev->name) because it's under the device_list_mutex.
> 
> The locking around devices and related structures is not exactly
> straightforward, but here I don't think we need to stick to the RCU
> pattern if the protection is guaranteed by other means. This applies to
> uuid_mutex and device_list_mutex.

You're right, it's a little confusing because uuid_mutex protects
device->name, so under device_list_mutex, device->name might change but
will never become NULL. We can just drop this one, thanks!

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

end of thread, other threads:[~2017-09-07 17:24 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-23  6:45 [PATCH 0/7] Btrfs: bugs found by sparse and RCU strings Omar Sandoval
2017-08-23  6:45 ` [PATCH 1/7] Btrfs: fix blk_status_t/errno confusion Omar Sandoval
2017-08-23 15:50   ` David Sterba
2017-08-23 16:13   ` Liu Bo
2017-08-23 19:18     ` Omar Sandoval
2017-08-23  6:46 ` [PATCH 2/7] Btrfs: fix incorrect {node,sector}size endianness from BTRFS_IOC_FS_INFO Omar Sandoval
2017-09-06 14:51   ` David Sterba
2017-08-23  6:46 ` [PATCH 3/7] Move Btrfs RCU string to common library Omar Sandoval
2017-09-06 15:15   ` David Sterba
2017-09-07 17:10     ` Omar Sandoval
2017-08-23  6:46 ` [PATCH 4/7] Btrfs: refactor btrfs_device->name updates Omar Sandoval
2017-08-23  6:46 ` [PATCH 5/7] Btrfs: fix suspicious RCU in BTRFS_IOC_DEV_INFO Omar Sandoval
2017-09-06 15:37   ` David Sterba
2017-09-07 17:24     ` Omar Sandoval
2017-08-23  6:46 ` [PATCH 6/7] Btrfs: make some volumes.c functions static Omar Sandoval
2017-09-06 15:25   ` David Sterba
2017-08-23  6:46 ` [PATCH 7/7] Btrfs: fix __user casting in ioctl.c Omar Sandoval
2017-09-06 15:23   ` David Sterba

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).