Re: [PATCH] libfrog: fix the if condition in xfrog_bulk_req_v1_setup

["Darrick J. Wong" <djwong@xxxxxxxxxx>]

On Sat, Jul 30, 2022 at 09:25:25AM +0800, Stephen Zhang wrote:
> Darrick J. Wong <djwong@xxxxxxxxxx> 于2022年7月29日周五 23:44写道：
> >
> > On Fri, Jul 29, 2022 at 03:57:46PM +0800, Stephen Zhang wrote:
> > > when scanning all inodes in each ag, hdr->ino serves as a iterator to
> > > specify the ino to start scanning with.
> > >
> > > After hdr->ino-- , we can get the last ino returned from the previous
> > > iteration.
> > >
> > > But there are cases that hdr->ino-- is pointless, that is,the case when
> > > starting to scan inodes in each ag.
> > >
> > > Hence the condition should be cvt_ino_to_agno(xfd, hdr->ino) ==0, which
> > > represents the start of scan in each ag,
> >
> > Er, cvt_ino_to_agno extracts the AG number from an inumber;
> > cvt_ino_to_agino extracts the inumber within an AG.  Given your
> > description of the problem (not wanting hdr->ino to go backwards in the
> > inumber space when it's already at the start of an AG), I think you want
> > the latter here?
> >
> > > instead of hdr->ino ==0, which represents the start of scan in ag 0 only.
> > >
> > > Signed-off-by: Stephen Zhang <zhangshida@xxxxxxxxxx>
> > > ---
> > >  libfrog/bulkstat.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libfrog/bulkstat.c b/libfrog/bulkstat.c
> > > index 195f6ea0..77a385bb 100644
> > > --- a/libfrog/bulkstat.c
> > > +++ b/libfrog/bulkstat.c
> > > @@ -172,7 +172,7 @@ xfrog_bulk_req_v1_setup(
> > >       if (!buf)
> > >               return -errno;
> > >
> > > -     if (hdr->ino)
> > > +     if (cvt_ino_to_agno(xfd, hdr->ino))
> >
> > ...because I think this change means that we go backwards for any inode
> > in AG 0, and we do not go backwards for any other AG.
> >
> > --D
> >
> > >               hdr->ino--;
> > >       bulkreq->lastip = (__u64 *)&hdr->ino,
> > >       bulkreq->icount = hdr->icount,
> > > --
> > > 2.25.1
> > >
> 
> Yeah, i mean the latter. Sorry for the mistake.
> Hence the patch will be like:
> =====
> @@ -172,7 +172,7 @@ xfrog_bulk_req_v1_setup(
>         if (!buf)
>                 return -errno;
> 
> -       if (hdr->ino)
> +       if (cvt_ino_to_agino(xfd, hdr->ino))
>                 hdr->ino--;
>         bulkreq->lastip = (__u64 *)&hdr->ino,
>         bulkreq->icount = hdr->icount,
> ====
> Should i resend the patch later, or do you have any other idea about
> this change?

It's probably ok to resend with that change, but ... what were you doing
to trip over this error, anyway?

--D

> Thanks,
> 
> Stephen.