On Tue, Jan 19, 2021 at 04:51:45PM +0100, David Sterba wrote:
> On Tue, Jan 19, 2021 at 06:54:28AM +0800, Qu Wenruo wrote:
> > On 2021/1/19 上午6:46, David Sterba wrote:
> > > On Sat, Jan 16, 2021 at 03:15:18PM +0800, Qu Wenruo wrote:
> > >> +                return;
> > >> +
> > >> +        subpage = (struct btrfs_subpage *)detach_page_private(page);
> > >> +        ASSERT(subpage);
> > >> +        kfree(subpage);
> > >> +}
> > >> diff --git a/fs/btrfs/subpage.h b/fs/btrfs/subpage.h
> > >> new file mode 100644
> > >> index 000000000000..96f3b226913e
> > >> --- /dev/null
> > >> +++ b/fs/btrfs/subpage.h
> > >> @@ -0,0 +1,31 @@
> > >> +/* SPDX-License-Identifier: GPL-2.0 */
> > >> +
> > >> +#ifndef BTRFS_SUBPAGE_H
> > >> +#define BTRFS_SUBPAGE_H
> > >> +
> > >> +#include <linux/spinlock.h>
> > >> +#include "ctree.h"
> > >
> > > So subpage.h would pull the whole ctree.h, that's not very nice. If
> > > anything, the .c could include ctree.h because there are lots of the
> > > common structure and function definitions, but not the .h. This creates
> > > unnecessary include dependencies.
> > >
> > > Any pointer type you'd need in structures could be forward declared.
> > 
> > Unfortunately, the main needed pointer is fs_info, and we're accessing
> > it pretty frequently (mostly for sector/node size).
> > 
> > I don't believe forward declaration would help in this case.
> 
> I've looked at the final subpage.h and you add way too many static
> inlines that don't seem to be necessary for the reasons the static
> inlines are supposed to be used.

The only file that includes subpage.h is extent_io.c, so as long as it
stays like that it's manageable. But untangling the include hell still
needs to hapen some day and new code that makes it harder worries me.

Reply via email to