On Mon, Feb 11, 2019 at 10:35:00AM +0200, Nikolay Borisov wrote:
> From: Jeff Mahoney <jeffm@xxxxxxxx>
>
> We currently overload the pending_chunks list to handle updating
> btrfs_device->commit_bytes used. We don't actually care about
> the extent mapping or even the device mapping for the chunk - we
> just need the device, and we can end up processing it multiple
> times. The fs_devices->resized_list does more or less the same
> thing, but with the disk size. They are called consecutively
> during commit and have more or less the same purpose.
>
> We can combine the two lists into a single list that attaches
> to the transaction and contains a list of devices that need
> updating. Since we always add the device to a list when we
> change bytes_used or disk_total_size, there's no harm in
> copying both values at once.
Agreed, overall sounds good.
> Signed-off-by: Jeff Mahoney <jeffm@xxxxxxxx>
> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
> ---
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -75,6 +75,7 @@ void btrfs_put_transaction(struct btrfs_transaction *transaction)
> btrfs_put_block_group_trimming(cache);
> btrfs_put_block_group(cache);
> }
> + BUG_ON(!list_empty(&transaction->dev_update_list));
Why BUG_ON? Would ASSERT or WARN_ON enough?
> kfree(transaction);
> }
> }
> @@ -334,6 +333,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid,
>
> void btrfs_free_device(struct btrfs_device *device)
> {
> + BUG_ON(!list_empty(&device->post_commit_list));
Same here
> rcu_string_free(device->name);
> bio_put(device->flush_bio);
> kfree(device);
> + /*
> + * We don't need the device_list_mutex here. This list is owned
> + * by the transaction and the transaction must complete before
> + * the device is released.
> + */
> + mutex_lock(&trans->fs_info->chunk_mutex);
> + list_for_each_entry_safe(curr, next, &trans->dev_update_list,
> + post_commit_list) {
> + list_del_init(&curr->post_commit_list);
> + curr->commit_total_bytes = curr->disk_total_bytes;
> + curr->commit_bytes_used = curr->bytes_used;
Agreed, the chunk_mutex should be enough from what I've seen.
> }
> - mutex_unlock(&fs_info->chunk_mutex);
> + mutex_unlock(&trans->fs_info->chunk_mutex);
> }
>
> void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info)
