Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221205175140.1543229-1-nphamcs@gmail.com> <20221205175140.1543229-4-nphamcs@gmail.com>
 <Y5UbhBTB2nSMN4UD@ZenIV> <CAKEwX=NwUPShF3zud7kn_gyd4BZy8S6xzm6iFuw0eW=o=3A8nw@mail.gmail.com>
 <Y5dV2jrZGMvgIZCx@casper.infradead.org>
In-Reply-To: <Y5dV2jrZGMvgIZCx@casper.infradead.org>
From:   Nhat Pham <nphamcs@gmail.com>
Date:   Mon, 12 Dec 2022 08:37:39 -0800
Message-ID: <CAKEwX=MDEE__cQOiDTA1q9s3RFbV6N+pzE=npufmtZ3vdxxGgw@mail.gmail.com>
Subject: Re: [PATCH v2 3/4] cachestat: implement cachestat syscall
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Al Viro <viro@zeniv.linux.org.uk>, akpm@linux-foundation.org,
        hannes@cmpxchg.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, bfoster@redhat.com,
        kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Dec 12, 2022 at 8:24 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Dec 12, 2022 at 08:23:31AM -0800, Nhat Pham wrote:
> > > It would be easier to read if you inverted the condition here.
> >
> > Oh I think I tried
> >
> > if (!f.file)
> >        return -EBADF;
> >
> > here, but there are some mixing-code-with-decl warnings.
> > If I recall correctly, the problem is with this line:
> >
> > XA_STATE(xas, &mapping->i_pages, first_index);
> >
> > which is expanded into a declaration:
> >
> > #define XA_STATE(name, array, index) \
> > struct xa_state name = __XA_STATE(array, index, 0, 0)
> >
> > It requires a valid mapping though, which is
> > obtained from f.file:
> >
> > struct address_space *mapping = f.file->f_mapping;
> >
> > so it cannot be moved above the if(!f.file) check either...
>
> Perhaps you're trying to do too much in a single function?

That's fair. In an ancient version of this, I did try to separate
the folio-walking logic into its own function, but then decided
against it because it seemed unnecessary. That was before
the complexity of that piece of logic blew up to the current
situation though, so perhaps it is time to revisit this decision
and refactor it.