| Message ID | b5ad15e6583f4e61cfd44344ef17ea7a93f6bb57.1598792561.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: seed fix null ptr, use only main device_list_mutex, and cleanups | expand |
On 30.08.20 г. 17:40 ч., Anand Jain wrote: > On an fs mounted using a sprout-device, the seed fs_devices are maintained > in a linked list under fs_info->fs_devices. Each seed's fs_devices also > have device_list_mutex initialized to protect against the potential race > with delete threads. But the delete thread (at btrfs_rm_device()) is holding > the fs_info::fs_devices::device_list_mutex mutex which is sprout's > device_list_mutex instead of seed's device_list_mutex. Moreover, there > aren't any significient benefits in using the seed::device_list_mutex > instead of sprout::device_list_mutex. > > So this patch converts them of using the seed::device_list_mutex to > sprout::device_list_mutex. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> nit: I'm beginning to think that the structure representing seed devices shouldn't be a full-fledged btrfs_fs_devices but a slimmed down version which contains only the necessary bits. Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/reada.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c > index c0035fc0ec67..9b54a51ba860 100644 > --- a/fs/btrfs/reada.c > +++ b/fs/btrfs/reada.c > @@ -776,13 +776,11 @@ static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices) > > do { > enqueued = 0; > - mutex_lock(&fs_devices->device_list_mutex); > list_for_each_entry(device, &fs_devices->devices, dev_list) { > if (atomic_read(&device->reada_in_flight) < > MAX_IN_FLIGHT) > enqueued += reada_start_machine_dev(device); > } > - mutex_unlock(&fs_devices->device_list_mutex); > total += enqueued; > } while (enqueued && total < 10000); > > @@ -795,10 +793,13 @@ static void __reada_start_machine(struct btrfs_fs_info *fs_info) > int i; > u64 enqueued = 0; > > + mutex_lock(&fs_devices->device_list_mutex); > + > enqueued += __reada_start_for_fsdevs(fs_devices); > list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) > enqueued += __reada_start_for_fsdevs(seed_devs); > > + mutex_unlock(&fs_devices->device_list_mutex); > if (enqueued == 0) > return; > >
On 8/30/20 10:40 AM, Anand Jain wrote: > On an fs mounted using a sprout-device, the seed fs_devices are maintained > in a linked list under fs_info->fs_devices. Each seed's fs_devices also > have device_list_mutex initialized to protect against the potential race > with delete threads. But the delete thread (at btrfs_rm_device()) is holding > the fs_info::fs_devices::device_list_mutex mutex which is sprout's > device_list_mutex instead of seed's device_list_mutex. Moreover, there > aren't any significient benefits in using the seed::device_list_mutex > instead of sprout::device_list_mutex. > > So this patch converts them of using the seed::device_list_mutex to > sprout::device_list_mutex. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> This doesn't apply cleanly to misc-next as of this morning. Thanks, Josef
On 1/9/20 12:08 am, Josef Bacik wrote: > On 8/30/20 10:40 AM, Anand Jain wrote: >> On an fs mounted using a sprout-device, the seed fs_devices are >> maintained >> in a linked list under fs_info->fs_devices. Each seed's fs_devices also >> have device_list_mutex initialized to protect against the potential race >> with delete threads. But the delete thread (at btrfs_rm_device()) is >> holding >> the fs_info::fs_devices::device_list_mutex mutex which is sprout's >> device_list_mutex instead of seed's device_list_mutex. Moreover, there >> aren't any significient benefits in using the seed::device_list_mutex >> instead of sprout::device_list_mutex. >> >> So this patch converts them of using the seed::device_list_mutex to >> sprout::device_list_mutex. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > This doesn't apply cleanly to misc-next as of this morning. Thanks, Kdave renamed the function to reada_start_for_fsdevs() dropping the __ prefix. Rebased my misc-next. I am sending V2. Thanks, Anand > > Josef
diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c index c0035fc0ec67..9b54a51ba860 100644 --- a/fs/btrfs/reada.c +++ b/fs/btrfs/reada.c @@ -776,13 +776,11 @@ static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices) do { enqueued = 0; - mutex_lock(&fs_devices->device_list_mutex); list_for_each_entry(device, &fs_devices->devices, dev_list) { if (atomic_read(&device->reada_in_flight) < MAX_IN_FLIGHT) enqueued += reada_start_machine_dev(device); } - mutex_unlock(&fs_devices->device_list_mutex); total += enqueued; } while (enqueued && total < 10000); @@ -795,10 +793,13 @@ static void __reada_start_machine(struct btrfs_fs_info *fs_info) int i; u64 enqueued = 0; + mutex_lock(&fs_devices->device_list_mutex); + enqueued += __reada_start_for_fsdevs(fs_devices); list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) enqueued += __reada_start_for_fsdevs(seed_devs); + mutex_unlock(&fs_devices->device_list_mutex); if (enqueued == 0) return;
On an fs mounted using a sprout-device, the seed fs_devices are maintained in a linked list under fs_info->fs_devices. Each seed's fs_devices also have device_list_mutex initialized to protect against the potential race with delete threads. But the delete thread (at btrfs_rm_device()) is holding the fs_info::fs_devices::device_list_mutex mutex which is sprout's device_list_mutex instead of seed's device_list_mutex. Moreover, there aren't any significient benefits in using the seed::device_list_mutex instead of sprout::device_list_mutex. So this patch converts them of using the seed::device_list_mutex to sprout::device_list_mutex. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/reada.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)