On Fri, 2024-12-06 at 17:05 +1100, NeilBrown wrote:
> On Fri, 06 Dec 2024, Jeff Layton wrote:
> > On Fri, 2024-12-06 at 11:43 +1100, NeilBrown wrote:
>
> > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > > index 382cc1389396..c26ba86dbdfd 100644
> > > --- a/fs/nfsd/xdr4.h
> > > +++ b/fs/nfsd/xdr4.h
> > > @@ -576,9 +576,7 @@ struct nfsd4_sequence {
> > > u32 slotid; /* request/response */
> > > u32 maxslots; /* request/response */
> > > u32 cachethis; /* request */
> > > -#if 0
> > > u32 target_maxslots; /* response */
> > > -#endif /* not yet */
> > > u32 status_flags; /* response */
> > > };
> > >
> >
> >
> > I don't see where the above "#if 0" gets removed in patch 6. Shouldn't
> > it be?
>
> You are misreading. It is being removed here in patch 5.
> It was added in 2.6.38 in
> Commit b85d4c01b76f ("nfsd41: sequence operation")
>
Oh, sorry -- my mistake. That's what I get for reviewing patches just
before boarding a redeye flight!
>
> >
> > While it makes for a larger patch, I think we'd be better served by
> > squashing 5 and 6 together. It doesn't make sense to add this core
> > infrastructure without something to call it.
>
> I find it easier to review if the distinct elements of functionality are
> kept separate. But if both you and Chuck want just one patch here, I
> can do that.
>
The proposed code is bisectable, so I don't feel too strongly about it.
Adding in unused functions is "Not The Way We (Usually) Do Things"
though.
I think in this case it was harder for me to review, since I had to
skip ahead to patch #6 to see how reduce_session_slots() would actually
be used. The spin_trylock(), in particular was confusing until I
realized it was being called from a shrinker that iterated over all of
the clients and spinning there is probably not good.
Either way, a kerneldoc header over reduce_session_slots() that
explains this subtlety would be nice.
--
Jeff Layton <jlayton@xxxxxxxxxx>
