Re: [PATCH v2 2/2] btrfs: qgroup: add sysfs interface for debug

From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <wqu@suse.com>
Cc: dsterba@suse.cz, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v2 2/2] btrfs: qgroup: add sysfs interface for debug
Date: Tue, 30 Jun 2020 10:07:35 +0200	[thread overview]
Message-ID: <20200630080735.GQ27795@twin.jikos.cz> (raw)
In-Reply-To: <9af22db7-42f0-00fd-1a34-33d6a8cffc2f@suse.com>

On Tue, Jun 30, 2020 at 07:17:25AM +0800, Qu Wenruo wrote:
> On 2020/6/30 上午5:30, David Sterba wrote:
> > On Sun, Jun 28, 2020 at 01:07:15PM +0800, Qu Wenruo wrote:
> >> +QGROUP_ATTR(rfer, reference);
> > 
> > Note that this is 'referenced'.
> > 
> >> +QGROUP_ATTR(excl, exclusive);
> >> +QGROUP_ATTR(max_rfer, max_reference);
> > 
> > And here max_referenced.
> > 
> >> +QGROUP_ATTR(max_excl, max_exclusive);
> >> +QGROUP_ATTR(lim_flags, limit_flags);
> >> +QGROUP_RSV_ATTR(data, BTRFS_QGROUP_RSV_DATA);
> >> +QGROUP_RSV_ATTR(meta_pertrans, BTRFS_QGROUP_RSV_META_PERTRANS);
> >> +QGROUP_RSV_ATTR(meta_prealloc, BTRFS_QGROUP_RSV_META_PREALLOC);
> > 
> > The two above fixed but otherwise it's good, thanks.
> > 
> > The qgroup membership and relations could be added to the sysfs export
> > too, but we're limited by the PAGE_SIZE output buffer so the information
> > could be incomplete.
> > 
> Yep, PAGE_SIZE is one limitation.
> But we can also go another direction, just using a new dir for related
> qgroups, then we can workaround the limitation.
>
> But personally speaking, the main objective is still the rsv_* members
> for debug.
> I don't really want to turn the sysfs interface into a way to export all
> qgroup info yet.

Well, that's a bit of a problem, if you're designing a public interface
for your own needs and neglecting that it will be used as a way to read
qgroup information. Sooner or later someone will ask for the additional
information to be added.

If it's just for debugging then it should be under 'debug', but as the
bug reporter does not want to apply patches/change config you ask to
make it visible unconditionally. I'm ok with that as long as the
interface is done right because any mistake will be potentially much
harder to fix in the future.

We need to at lest have a solid idea how to extend it, not neccessarily
to implement it right now. Adding the group membership to qgroup
directories under the qgroup directory as symlinks between the kobjects
sounds ok-ish to me but it's a new idea and I need to think about it.