2024-03-01 19:27:14

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH v2 03/13] filemap: align the index to mapping_min_order in the page cache

On Fri, Mar 01, 2024 at 05:44:34PM +0100, Pankaj Raghav (Samsung) wrote:
> +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \
> + struct readahead_control ractl = { \
> + .file = f, \
> + .mapping = m, \
> + .ra = r, \
> + ._index = mapping_align_start_index(m, i), \
> + }

My point was that you didn't need to do any of this.

Look, I've tried to give constructive review, but I feel like I'm going
to have to be blunt. There is no evidence of design or understanding
in these patches or their commit messages. You don't have a coherent
message about "These things have to be aligned; these things can be at
arbitrary alignment". If you have thought about it, it doesn't show.

Maybe you just need to go back over the patches and read them as a series,
but it feels like "Oh, there's a hole here, patch it; another hole here,
patch it" without thinking about what's going on and why.

I want to help, but it feels like it'd be easier to do all the work myself
at this point, and that's not good for me, and it's not good for you.

So, let's start off: Is the index in ractl aligned or not, and why do
you believe that's the right approach? And review each of the patches
in this series with the answer to that question in mind because you are
currently inconsistent.


2024-03-01 20:05:08

by Kent Overstreet

[permalink] [raw]
Subject: Re: [PATCH v2 03/13] filemap: align the index to mapping_min_order in the page cache

On Fri, Mar 01, 2024 at 07:26:55PM +0000, Matthew Wilcox wrote:
> On Fri, Mar 01, 2024 at 05:44:34PM +0100, Pankaj Raghav (Samsung) wrote:
> > +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \
> > + struct readahead_control ractl = { \
> > + .file = f, \
> > + .mapping = m, \
> > + .ra = r, \
> > + ._index = mapping_align_start_index(m, i), \
> > + }
>
> My point was that you didn't need to do any of this.
>
> Look, I've tried to give constructive review, but I feel like I'm going
> to have to be blunt. There is no evidence of design or understanding
> in these patches or their commit messages. You don't have a coherent
> message about "These things have to be aligned; these things can be at
> arbitrary alignment". If you have thought about it, it doesn't show.

Don't you think you might be going off a bit much? I looked over these
patches after we talked privately, and they looked pretty sensible to
me...

Yes, we _always_ want more thorough commit messages that properly
explain the motivations for changes, but in my experience that's the
thing that takes the longest to learn how to do well as an engineer...
ease up abit.

> So, let's start off: Is the index in ractl aligned or not, and why do
> you believe that's the right approach? And review each of the patches
> in this series with the answer to that question in mind because you are
> currently inconsistent.

^ this is a real point though, DEFINE_READAHEAD_ALIGNED() feels off to
me.

2024-03-05 10:01:22

by Pankaj Raghav (Samsung)

[permalink] [raw]
Subject: Re: [PATCH v2 03/13] filemap: align the index to mapping_min_order in the page cache

On Fri, Mar 01, 2024 at 07:26:55PM +0000, Matthew Wilcox wrote:
> On Fri, Mar 01, 2024 at 05:44:34PM +0100, Pankaj Raghav (Samsung) wrote:
> > +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \
> > + struct readahead_control ractl = { \
> > + .file = f, \
> > + .mapping = m, \
> > + .ra = r, \
> > + ._index = mapping_align_start_index(m, i), \
> > + }
>
> My point was that you didn't need to do any of this.
>
Got it. I probably didn't understand your old comment properly.

> Look, I've tried to give constructive review, but I feel like I'm going
> to have to be blunt. There is no evidence of design or understanding
> in these patches or their commit messages. You don't have a coherent
> message about "These things have to be aligned; these things can be at
> arbitrary alignment". If you have thought about it, it doesn't show.
>
> Maybe you just need to go back over the patches and read them as a series,
> but it feels like "Oh, there's a hole here, patch it; another hole here,
> patch it" without thinking about what's going on and why.
>
> I want to help, but it feels like it'd be easier to do all the work myself
> at this point, and that's not good for me, and it's not good for you.
>
> So, let's start off: Is the index in ractl aligned or not, and why do
> you believe that's the right approach? And review each of the patches
> in this series with the answer to that question in mind because you are
> currently inconsistent.

Thanks for the feedback, and I get your comment about inconsistentency,
especially in the part where we align the index probably in places where
it doesn't even matter. As someone who is a bit new to the inner
workings of the page cache, I was a bit unsure about choosing the right
abstracation to enforce alignment.

I am going through all the patches now based on your feedback and
changing the commit messages to clarify the intent.

--
Pankaj

2024-03-05 10:05:33

by Pankaj Raghav (Samsung)

[permalink] [raw]
Subject: Re: [PATCH v2 03/13] filemap: align the index to mapping_min_order in the page cache

On Fri, Mar 01, 2024 at 03:04:33PM -0500, Kent Overstreet wrote:
> On Fri, Mar 01, 2024 at 07:26:55PM +0000, Matthew Wilcox wrote:
> > On Fri, Mar 01, 2024 at 05:44:34PM +0100, Pankaj Raghav (Samsung) wrote:
> > > +#define DEFINE_READAHEAD_ALIGNED(ractl, f, r, m, i) \
> > > + struct readahead_control ractl = { \
> > > + .file = f, \
> > > + .mapping = m, \
> > > + .ra = r, \
> > > + ._index = mapping_align_start_index(m, i), \
> > > + }
> >
> > My point was that you didn't need to do any of this.
> >
> > Look, I've tried to give constructive review, but I feel like I'm going
> > to have to be blunt. There is no evidence of design or understanding
> > in these patches or their commit messages. You don't have a coherent
> > message about "These things have to be aligned; these things can be at
> > arbitrary alignment". If you have thought about it, it doesn't show.
>
> Don't you think you might be going off a bit much? I looked over these
> patches after we talked privately, and they looked pretty sensible to
> me...
>
> Yes, we _always_ want more thorough commit messages that properly
> explain the motivations for changes, but in my experience that's the
> thing that takes the longest to learn how to do well as an engineer...
> ease up abit.
>
> > So, let's start off: Is the index in ractl aligned or not, and why do
> > you believe that's the right approach? And review each of the patches
> > in this series with the answer to that question in mind because you are
> > currently inconsistent.
>
> ^ this is a real point though, DEFINE_READAHEAD_ALIGNED() feels off to
> me.

Thanks Kent. I am going over the patches again and changing it based on
the feedback.