| Message ID | 20210209203041.21493-5-mrostecki@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add roundrobin raid1 read policy | expand |
On Tue, Feb 09, 2021 at 09:30:38PM +0100, Michal Rostecki wrote: > From: Michal Rostecki <mrostecki@suse.com> > > Add the btrfs_check_mixed() function which checks if the filesystem has > the mixed type of devices (non-rotational and rotational). This > information is going to be used in roundrobin raid1 read policy.a [...] > @@ -669,8 +699,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > } > > q = bdev_get_queue(bdev); > - if (!blk_queue_nonrot(q)) > + rotating = !blk_queue_nonrot(q); > + device->rotating = rotating; > + if (rotating) > fs_devices->rotating = true; > + if (!fs_devices->mixed) > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); [...] Since this is adding to a set, a faster way is: if (fs_devices->rotating != rotating) fs_devices->mixed = true; The scan might be necessary on device removal, though. > - if (!blk_queue_nonrot(q)) > + rotating = !blk_queue_nonrot(q); > + device->rotating = rotating; > + if (rotating) > fs_devices->rotating = true; > + if (!fs_devices->mixed) > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); Duplication. Maybe pull all this into a function? Best Regards, Michał Mirosław
On Tue, Feb 9, 2021 at 9:32 PM Michal Rostecki <mrostecki@suse.de> wrote: > > From: Michal Rostecki <mrostecki@suse.com> > > Add the btrfs_check_mixed() function which checks if the filesystem has > the mixed type of devices (non-rotational and rotational). This > information is going to be used in roundrobin raid1 read policy. > > Signed-off-by: Michal Rostecki <mrostecki@suse.com> > --- > fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > fs/btrfs/volumes.h | 7 +++++++ > 2 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 1ac364a2f105..1ad30a595722 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -617,6 +617,35 @@ static int btrfs_free_stale_devices(const char *path, > return ret; > } > > +/* > + * Checks if after adding the new device the filesystem is going to have mixed > + * types of devices (non-rotational and rotational). > + * > + * @fs_devices: list of devices > + * @new_device_rotating: if the new device is rotational > + * > + * Returns true if there are mixed types of devices, otherwise returns false. > + */ > +static bool btrfs_check_mixed(struct btrfs_fs_devices *fs_devices, > + bool new_device_rotating) > +{ > + struct btrfs_device *device, *prev_device; > + > + list_for_each_entry(device, &fs_devices->devices, dev_list) { > + if (prev_device == NULL && Hum, prev_device is not initialized when we enter the first iteration of the loop. > + device->rotating != new_device_rotating) > + return true; > + if (prev_device != NULL && > + (device->rotating != prev_device->rotating || Here it's more dangerous, dereferencing an uninitialized pointer can result in a crash. With this fixed, it would be better to redo the benchmarks when using mixed device types. Thanks. > + device->rotating != new_device_rotating)) > + return true; > + > + prev_device = device; > + } > + > + return false; > +} > + > /* > * This is only used on mount, and we are protected from competing things > * messing with our fs_devices by the uuid_mutex, thus we do not need the > @@ -629,6 +658,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > struct request_queue *q; > struct block_device *bdev; > struct btrfs_super_block *disk_super; > + bool rotating; > u64 devid; > int ret; > > @@ -669,8 +699,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > } > > q = bdev_get_queue(bdev); > - if (!blk_queue_nonrot(q)) > + rotating = !blk_queue_nonrot(q); > + device->rotating = rotating; > + if (rotating) > fs_devices->rotating = true; > + if (!fs_devices->mixed) > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > > device->bdev = bdev; > clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state); > @@ -2418,6 +2452,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) > fs_devices->open_devices = 0; > fs_devices->missing_devices = 0; > fs_devices->rotating = false; > + fs_devices->mixed = false; > list_add(&seed_devices->seed_list, &fs_devices->seed_list); > > generate_random_uuid(fs_devices->fsid); > @@ -2522,6 +2557,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path > int seeding_dev = 0; > int ret = 0; > bool locked = false; > + bool rotating; > > if (sb_rdonly(sb) && !fs_devices->seeding) > return -EROFS; > @@ -2621,8 +2657,12 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path > > atomic64_add(device->total_bytes, &fs_info->free_chunk_space); > > - if (!blk_queue_nonrot(q)) > + rotating = !blk_queue_nonrot(q); > + device->rotating = rotating; > + if (rotating) > fs_devices->rotating = true; > + if (!fs_devices->mixed) > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > > orig_super_total_bytes = btrfs_super_total_bytes(fs_info->super_copy); > btrfs_set_super_total_bytes(fs_info->super_copy, > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index 6e544317a377..594f1207281c 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -147,6 +147,9 @@ struct btrfs_device { > /* I/O stats for raid1 mirror selection */ > struct percpu_counter inflight; > atomic_t last_offset; > + > + /* If the device is rotational */ > + bool rotating; > }; > > /* > @@ -274,6 +277,10 @@ struct btrfs_fs_devices { > * nonrot flag set > */ > bool rotating; > + /* Set when we find or add both nonrot and rot disks in the > + * filesystem > + */ > + bool mixed; > > struct btrfs_fs_info *fs_info; > /* sysfs kobjects */ > -- > 2.30.0 >
On Wed, Feb 10, 2021 at 05:08:05AM +0100, Michał Mirosław wrote: > On Tue, Feb 09, 2021 at 09:30:38PM +0100, Michal Rostecki wrote: > > From: Michal Rostecki <mrostecki@suse.com> > > > > Add the btrfs_check_mixed() function which checks if the filesystem has > > the mixed type of devices (non-rotational and rotational). This > > information is going to be used in roundrobin raid1 read policy.a > [...] > > @@ -669,8 +699,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > > } > > > > q = bdev_get_queue(bdev); > > - if (!blk_queue_nonrot(q)) > > + rotating = !blk_queue_nonrot(q); > > + device->rotating = rotating; > > + if (rotating) > > fs_devices->rotating = true; > > + if (!fs_devices->mixed) > > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > [...] > > Since this is adding to a set, a faster way is: > > if (fs_devices->rotating != rotating) > fs_devices->mixed = true; > Good idea. > The scan might be necessary on device removal, though. > And good point. I didn't address the case of device removal at all. > > - if (!blk_queue_nonrot(q)) > > + rotating = !blk_queue_nonrot(q); > > + device->rotating = rotating; > > + if (rotating) > > fs_devices->rotating = true; > > + if (!fs_devices->mixed) > > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > > Duplication. Maybe pull all this into a function? > Will do that as well. > Best Regards, > Michał Mirosław Regards, Michal
On Wed, Feb 10, 2021 at 10:09:10AM +0000, Filipe Manana wrote: > On Tue, Feb 9, 2021 at 9:32 PM Michal Rostecki <mrostecki@suse.de> wrote: > > > > From: Michal Rostecki <mrostecki@suse.com> > > > > Add the btrfs_check_mixed() function which checks if the filesystem has > > the mixed type of devices (non-rotational and rotational). This > > information is going to be used in roundrobin raid1 read policy. > > > > Signed-off-by: Michal Rostecki <mrostecki@suse.com> > > --- > > fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > > fs/btrfs/volumes.h | 7 +++++++ > > 2 files changed, 49 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index 1ac364a2f105..1ad30a595722 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -617,6 +617,35 @@ static int btrfs_free_stale_devices(const char *path, > > return ret; > > } > > > > +/* > > + * Checks if after adding the new device the filesystem is going to have mixed > > + * types of devices (non-rotational and rotational). > > + * > > + * @fs_devices: list of devices > > + * @new_device_rotating: if the new device is rotational > > + * > > + * Returns true if there are mixed types of devices, otherwise returns false. > > + */ > > +static bool btrfs_check_mixed(struct btrfs_fs_devices *fs_devices, > > + bool new_device_rotating) > > +{ > > + struct btrfs_device *device, *prev_device; > > + > > + list_for_each_entry(device, &fs_devices->devices, dev_list) { > > + if (prev_device == NULL && > > Hum, prev_device is not initialized when we enter the first iteration > of the loop. > > > + device->rotating != new_device_rotating) > > + return true; > > + if (prev_device != NULL && > > + (device->rotating != prev_device->rotating || > > Here it's more dangerous, dereferencing an uninitialized pointer can > result in a crash. > > With this fixed, it would be better to redo the benchmarks when using > mixed device types. > > Thanks. > Thanks for pointing that out. Will fix and redo benchmarks for v2. > > + device->rotating != new_device_rotating)) > > + return true; > > + > > + prev_device = device; > > + } > > + > > + return false; > > +} > > + > > /* > > * This is only used on mount, and we are protected from competing things > > * messing with our fs_devices by the uuid_mutex, thus we do not need the > > @@ -629,6 +658,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > > struct request_queue *q; > > struct block_device *bdev; > > struct btrfs_super_block *disk_super; > > + bool rotating; > > u64 devid; > > int ret; > > > > @@ -669,8 +699,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > > } > > > > q = bdev_get_queue(bdev); > > - if (!blk_queue_nonrot(q)) > > + rotating = !blk_queue_nonrot(q); > > + device->rotating = rotating; > > + if (rotating) > > fs_devices->rotating = true; > > + if (!fs_devices->mixed) > > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > > > > device->bdev = bdev; > > clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state); > > @@ -2418,6 +2452,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) > > fs_devices->open_devices = 0; > > fs_devices->missing_devices = 0; > > fs_devices->rotating = false; > > + fs_devices->mixed = false; > > list_add(&seed_devices->seed_list, &fs_devices->seed_list); > > > > generate_random_uuid(fs_devices->fsid); > > @@ -2522,6 +2557,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path > > int seeding_dev = 0; > > int ret = 0; > > bool locked = false; > > + bool rotating; > > > > if (sb_rdonly(sb) && !fs_devices->seeding) > > return -EROFS; > > @@ -2621,8 +2657,12 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path > > > > atomic64_add(device->total_bytes, &fs_info->free_chunk_space); > > > > - if (!blk_queue_nonrot(q)) > > + rotating = !blk_queue_nonrot(q); > > + device->rotating = rotating; > > + if (rotating) > > fs_devices->rotating = true; > > + if (!fs_devices->mixed) > > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > > > > orig_super_total_bytes = btrfs_super_total_bytes(fs_info->super_copy); > > btrfs_set_super_total_bytes(fs_info->super_copy, > > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > > index 6e544317a377..594f1207281c 100644 > > --- a/fs/btrfs/volumes.h > > +++ b/fs/btrfs/volumes.h > > @@ -147,6 +147,9 @@ struct btrfs_device { > > /* I/O stats for raid1 mirror selection */ > > struct percpu_counter inflight; > > atomic_t last_offset; > > + > > + /* If the device is rotational */ > > + bool rotating; > > }; > > > > /* > > @@ -274,6 +277,10 @@ struct btrfs_fs_devices { > > * nonrot flag set > > */ > > bool rotating; > > + /* Set when we find or add both nonrot and rot disks in the > > + * filesystem > > + */ > > + bool mixed; > > > > struct btrfs_fs_info *fs_info; > > /* sysfs kobjects */ > > -- > > 2.30.0 > > > > > -- > Filipe David Manana, > > “Whether you think you can, or you think you can't — you're right.”
On Wed, Feb 10, 2021 at 05:08:05AM +0100, Michał Mirosław wrote: > On Tue, Feb 09, 2021 at 09:30:38PM +0100, Michal Rostecki wrote: > > From: Michal Rostecki <mrostecki@suse.com> > > > > Add the btrfs_check_mixed() function which checks if the filesystem has > > the mixed type of devices (non-rotational and rotational). This > > information is going to be used in roundrobin raid1 read policy.a > [...] > > @@ -669,8 +699,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > > } > > > > q = bdev_get_queue(bdev); > > - if (!blk_queue_nonrot(q)) > > + rotating = !blk_queue_nonrot(q); > > + device->rotating = rotating; > > + if (rotating) > > fs_devices->rotating = true; > > + if (!fs_devices->mixed) > > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > [...] > > Since this is adding to a set, a faster way is: > > if (fs_devices->rotating != rotating) > fs_devices->mixed = true; > > The scan might be necessary on device removal, though. > Actually, that's not going to work in case of appenging a rotational device when all previous devices are non-rotational. if (rotating) fs_devices->rotating = true; if (fs_devices->rotating != rotating) fs_devices->mixed = true; If all devices are non-rotational, we start with the following attributes: fs_devices->rotating: false fs_devices->mixed: false Then, while appending a rotational disk, we have: rotating = true; if (rotating) // if (true) fs_devices->rotating = true; // overriding with `true` if (fs_devices->rotating != rotating) // if (true != true), which is false fs_devices->mixed = true; // NOT EXECUTED So we end up fs_devices->mixed being `false`, despite having a mixed array. Inverting the order of those `if` checks would break the other permuitations which start with rotational disks. Therefore, to cover all cases, I think we need a full check, always. Regards, Michal
On Fri, Feb 12, 2021 at 06:26:41PM +0000, Michal Rostecki wrote: > On Wed, Feb 10, 2021 at 05:08:05AM +0100, Michał Mirosław wrote: > > On Tue, Feb 09, 2021 at 09:30:38PM +0100, Michal Rostecki wrote: > > > From: Michal Rostecki <mrostecki@suse.com> > > > > > > Add the btrfs_check_mixed() function which checks if the filesystem has > > > the mixed type of devices (non-rotational and rotational). This > > > information is going to be used in roundrobin raid1 read policy.a > > [...] > > > @@ -669,8 +699,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > > > } > > > > > > q = bdev_get_queue(bdev); > > > - if (!blk_queue_nonrot(q)) > > > + rotating = !blk_queue_nonrot(q); > > > + device->rotating = rotating; > > > + if (rotating) > > > fs_devices->rotating = true; > > > + if (!fs_devices->mixed) > > > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > > [...] > > > > Since this is adding to a set, a faster way is: > > > > if (fs_devices->rotating != rotating) > > fs_devices->mixed = true; > > > > The scan might be necessary on device removal, though. > Actually, that's not going to work in case of appenging a rotational > device when all previous devices are non-rotational. [...] > Inverting the order of those `if` checks would break the other > permuitations which start with rotational disks. But not if you would add: if (adding first device) fs_devices->rotating = rotating; before the checks. But them, there is a simpler way: count how many rotating vs non-rotating devices there are while adding them. Like: rotating ? ++n_rotating : ++n_fixed; And then on remove you'd have it covered. Best Regards Michał Mirosław
On Sat, Feb 13, 2021 at 12:36:02AM +0100, Michał Mirosław wrote: > On Fri, Feb 12, 2021 at 06:26:41PM +0000, Michal Rostecki wrote: > > On Wed, Feb 10, 2021 at 05:08:05AM +0100, Michał Mirosław wrote: > > > On Tue, Feb 09, 2021 at 09:30:38PM +0100, Michal Rostecki wrote: > > > > From: Michal Rostecki <mrostecki@suse.com> > > > > > > > > Add the btrfs_check_mixed() function which checks if the filesystem has > > > > the mixed type of devices (non-rotational and rotational). This > > > > information is going to be used in roundrobin raid1 read policy.a > > > [...] > > > > @@ -669,8 +699,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, > > > > } > > > > > > > > q = bdev_get_queue(bdev); > > > > - if (!blk_queue_nonrot(q)) > > > > + rotating = !blk_queue_nonrot(q); > > > > + device->rotating = rotating; > > > > + if (rotating) > > > > fs_devices->rotating = true; > > > > + if (!fs_devices->mixed) > > > > + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); > > > [...] > > > > > > Since this is adding to a set, a faster way is: > > > > > > if (fs_devices->rotating != rotating) > > > fs_devices->mixed = true; > > > > > > The scan might be necessary on device removal, though. > > Actually, that's not going to work in case of appenging a rotational > > device when all previous devices are non-rotational. > [...] > > Inverting the order of those `if` checks would break the other > > permuitations which start with rotational disks. > > But not if you would add: > > if (adding first device) > fs_devices->rotating = rotating; > > before the checks. > > But them, there is a simpler way: count how many rotating vs non-rotating > devices there are while adding them. Like: > > rotating ? ++n_rotating : ++n_fixed; > > And then on remove you'd have it covered. I like the idea of storing numbers and simply checking them. I use it in v2 - though probably in a different form, and I will most likely move the whole logic around checking device types to separate functions, to not bloat btrfs_open_one_device() and the others too much.
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 1ac364a2f105..1ad30a595722 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -617,6 +617,35 @@ static int btrfs_free_stale_devices(const char *path, return ret; } +/* + * Checks if after adding the new device the filesystem is going to have mixed + * types of devices (non-rotational and rotational). + * + * @fs_devices: list of devices + * @new_device_rotating: if the new device is rotational + * + * Returns true if there are mixed types of devices, otherwise returns false. + */ +static bool btrfs_check_mixed(struct btrfs_fs_devices *fs_devices, + bool new_device_rotating) +{ + struct btrfs_device *device, *prev_device; + + list_for_each_entry(device, &fs_devices->devices, dev_list) { + if (prev_device == NULL && + device->rotating != new_device_rotating) + return true; + if (prev_device != NULL && + (device->rotating != prev_device->rotating || + device->rotating != new_device_rotating)) + return true; + + prev_device = device; + } + + return false; +} + /* * This is only used on mount, and we are protected from competing things * messing with our fs_devices by the uuid_mutex, thus we do not need the @@ -629,6 +658,7 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, struct request_queue *q; struct block_device *bdev; struct btrfs_super_block *disk_super; + bool rotating; u64 devid; int ret; @@ -669,8 +699,12 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices, } q = bdev_get_queue(bdev); - if (!blk_queue_nonrot(q)) + rotating = !blk_queue_nonrot(q); + device->rotating = rotating; + if (rotating) fs_devices->rotating = true; + if (!fs_devices->mixed) + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); device->bdev = bdev; clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state); @@ -2418,6 +2452,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) fs_devices->open_devices = 0; fs_devices->missing_devices = 0; fs_devices->rotating = false; + fs_devices->mixed = false; list_add(&seed_devices->seed_list, &fs_devices->seed_list); generate_random_uuid(fs_devices->fsid); @@ -2522,6 +2557,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path int seeding_dev = 0; int ret = 0; bool locked = false; + bool rotating; if (sb_rdonly(sb) && !fs_devices->seeding) return -EROFS; @@ -2621,8 +2657,12 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path atomic64_add(device->total_bytes, &fs_info->free_chunk_space); - if (!blk_queue_nonrot(q)) + rotating = !blk_queue_nonrot(q); + device->rotating = rotating; + if (rotating) fs_devices->rotating = true; + if (!fs_devices->mixed) + fs_devices->mixed = btrfs_check_mixed(fs_devices, rotating); orig_super_total_bytes = btrfs_super_total_bytes(fs_info->super_copy); btrfs_set_super_total_bytes(fs_info->super_copy, diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 6e544317a377..594f1207281c 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -147,6 +147,9 @@ struct btrfs_device { /* I/O stats for raid1 mirror selection */ struct percpu_counter inflight; atomic_t last_offset; + + /* If the device is rotational */ + bool rotating; }; /* @@ -274,6 +277,10 @@ struct btrfs_fs_devices { * nonrot flag set */ bool rotating; + /* Set when we find or add both nonrot and rot disks in the + * filesystem + */ + bool mixed; struct btrfs_fs_info *fs_info; /* sysfs kobjects */