Re: [PATCH] btrfs: free qgroup configuration when quota enable fails

Qu Wenruo <[email protected]>
Newsgroups org.kernel.vger.linux-btrfs,org.kernel.vger.linux-kernel
Message-ID <[email protected]>

在 2026/8/14 22:00, Guanjie Chen 写道:
> Hi Qu,
> 
>> We can reach this out branch if btrfs_start_transaction() failed, but
>> the fs_info->quota_root is already setup by another process.
> 
> Do you mean the window where qgroup_ioctl_lock is dropped before
> btrfs_start_transaction()?
> 
> I don't think another task can set fs_info->quota_root in that window.
> The quota ioctl holds fs_info->subvol_sem for writing across the whole
> btrfs_quota_enable() call, including while qgroup_ioctl_lock is dropped.
> 
> As far as I can see, quota_root is only set in two places:
> btrfs_quota_enable(), which is serialized by subvol_sem, and
> btrfs_read_roots(), which runs during mount initialization.

Right, I missed that rwsem.

But still, for error handling one should be in the reverse order.

And for out: label, there are cases we're going out label without 
creating any qgroup. In that case, qgroups removing should be done only 
for cases where a qgroup is created.

> 
> The disable path also calls btrfs_free_qgroup_config() and relies on the
> same subvol_sem and qgroup_ioctl_lock serialization for the qgroup
> configuration. If subvol_sem did not prevent another quota operation
> here, the disable path would have a similar lifetime problem.
> 
> So if quota_root was NULL at the first check, I believe it must still be
> NULL when btrfs_start_transaction() fails. I agree that the changelog
> should have mentioned subvol_sem as part of the protection.
> 
> Did I miss another runtime path that can set quota_root without holding
> subvol_sem?
> 
> Also, just to clarify, this is not my first btrfs contribution, although
> my previous change was only a small one.

The same, LLM disclosure if you're using it.

Nowadays I won't trust anyone who is not a regular contributor.

> 
> Thanks,
> Guanjie
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.