On Tue, Oct 17, 2023 at 10:31:27PM -0700, Darrick J. Wong wrote:
> On Wed, Oct 18, 2023 at 07:19:34AM +0200, Christoph Hellwig wrote:
> > On Tue, Oct 17, 2023 at 07:10:42PM -0700, Darrick J. Wong wrote:
> > > +static inline union xfs_suminfo_raw *
> > > xfs_rsumblock_infoptr(
> > > struct xfs_buf *bp,
> > > unsigned int index)
> > > {
> > > - xfs_suminfo_t *info = bp->b_addr;
> > > + union xfs_suminfo_raw *info = bp->b_addr;
> > >
> > > return info + index;
> > > }
> > >
> > > +/* Get the current value of a summary counter. */
> > > +static inline xfs_suminfo_t
> > > +xfs_suminfo_get(
> > > + struct xfs_buf *bp,
> > > + unsigned int index)
> > > +{
> > > + union xfs_suminfo_raw *info = xfs_rsumblock_infoptr(bp, index);
> > > +
> > > + return info->old;
> > > +}
> >
> > Same nitpick as for the bitmap version.
> >
> > Otherwise this looks good:
> >
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> Assuming you also meant "Reviewed-by" here too? :)
>
> > ... to actually understand the mess in xfs_rtmodify_summary_int I had
> > to do the (untested) refactoring below. I'll probably resubmit it after
> > the whole series which touches a lot of this:
>
> I'll take a look tomorrow, but yeah, xfs_rtmodify_summary_int is
> confusing.
>
> Annoyingly, I think there's a bug in new logging helpers, because all
> the shutdown tests are falling all over themselves:
>
> https://djwong.org/fstests/output/.c6b00f80b1e68bd5a3b17a7f7fbe97bab28dab740d7acf9e3fa879c3ae0c56c0/.02425ea8cdb6100e408b20dceac80a46f53f6fa1587fb4af7fba2810f2b8d0fd/?C=M;O=A
>
> Will look at /that/ first thing...
Oh rats, I forgot to adjust the xfs_trans_log_rtbitmap offsets for the
block headers added in the rtgroups patchset.
--D
> --D
>
> >
> > diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
> > index 2118c6f177a135..7b09caa747a720 100644
> > --- a/fs/xfs/libxfs/xfs_rtbitmap.c
> > +++ b/fs/xfs/libxfs/xfs_rtbitmap.c
> > @@ -433,78 +433,69 @@ xfs_trans_log_rtsummary(
> > * Summary information is returned in *sum if specified.
> > * If no delta is specified, returns summary only.
> > */
> > -int
> > -xfs_rtmodify_summary_int(
> > +static int
> > +xfs_rtmodify_summary_find(
> > xfs_mount_t *mp, /* file system mount structure */
> > xfs_trans_t *tp, /* transaction pointer */
> > int log, /* log2 of extent size */
> > xfs_fileoff_t bbno, /* bitmap block number */
> > - int delta, /* change to make to summary info */
> > struct xfs_buf **rbpp, /* in/out: summary block buffer */
> > xfs_fileoff_t *rsb, /* in/out: summary block number */
> > - xfs_suminfo_t *sum) /* out: summary info for this block */
> > + unsigned int *word)
> > {
> > struct xfs_buf *bp; /* buffer for the summary block */
> > int error; /* error value */
> > - xfs_fileoff_t sb; /* summary fsblock */
> > xfs_rtsumoff_t so; /* index into the summary file */
> > + xfs_fileoff_t sb; /* summary fsblock */
> >
> > /*
> > * Compute entry number in the summary file.
> > */
> > so = xfs_rtsumoffs(mp, log, bbno);
> > +
> > /*
> > * Compute the block number in the summary file.
> > */
> > sb = xfs_rtsumoffs_to_block(mp, so);
> > +
> > + /*
> > + * Compute the word index into the summary.
> > + */
> > + *word = xfs_rtsumoffs_to_infoword(mp, so);
> > +
> > /*
> > * If we have an old buffer, and the block number matches, use that.
> > */
> > if (*rbpp && *rsb == sb)
> > - bp = *rbpp;
> > + return 0;
> > +
> > /*
> > - * Otherwise we have to get the buffer.
> > + * Otherwise we have to get a new buffer.
> > + * If there was an old one, get rid of it first.
> > */
> > - else {
> > - /*
> > - * If there was an old one, get rid of it first.
> > - */
> > - if (*rbpp)
> > - xfs_trans_brelse(tp, *rbpp);
> > - error = xfs_rtbuf_get(mp, tp, sb, 1, &bp);
> > - if (error) {
> > - return error;
> > - }
> > - /*
> > - * Remember this buffer and block for the next call.
> > - */
> > - *rbpp = bp;
> > - *rsb = sb;
> > - }
> > + if (*rbpp)
> > + xfs_trans_brelse(tp, *rbpp);
> > + error = xfs_rtbuf_get(mp, tp, sb, 1, &bp);
> > + if (error)
> > + return error;
> > +
> > /*
> > - * Point to the summary information, modify/log it, and/or copy it out.
> > + * Remember this buffer and block for the next call.
> > */
> > - if (delta) {
> > - unsigned int infoword = xfs_rtsumoffs_to_infoword(mp, so);
> > - xfs_suminfo_t val = xfs_suminfo_add(bp, infoword, delta);
> > -
> > - if (mp->m_rsum_cache) {
> > - if (val == 0 && log == mp->m_rsum_cache[bbno])
> > - mp->m_rsum_cache[bbno]++;
> > - if (val != 0 && log < mp->m_rsum_cache[bbno])
> > - mp->m_rsum_cache[bbno] = log;
> > - }
> > - xfs_trans_log_rtsummary(tp, bp, infoword);
> > - if (sum)
> > - *sum = val;
> > - } else if (sum) {
> > - unsigned int infoword = xfs_rtsumoffs_to_infoword(mp, so);
> > -
> > - *sum = xfs_suminfo_get(bp, infoword);
> > - }
> > + *rbpp = bp;
> > + *rsb = sb;
> > return 0;
> > }
> >
> > +/*
> > + * Read and/or modify the summary information for a given extent size,
> > + * bitmap block combination.
> > + * Keeps track of a current summary block, so we don't keep reading
> > + * it from the buffer cache.
> > + *
> > + * Summary information is returned in *sum if specified.
> > + * If no delta is specified, returns summary only.
> > + */
> > int
> > xfs_rtmodify_summary(
> > xfs_mount_t *mp, /* file system mount structure */
> > @@ -515,8 +506,51 @@ xfs_rtmodify_summary(
> > struct xfs_buf **rbpp, /* in/out: summary block buffer */
> > xfs_fileoff_t *rsb) /* in/out: summary block number */
> > {
> > - return xfs_rtmodify_summary_int(mp, tp, log, bbno,
> > - delta, rbpp, rsb, NULL);
> > + int error;
> > + unsigned int word;
> > + xfs_suminfo_t val;
> > +
> > + error = xfs_rtmodify_summary_find(mp, tp, log, bbno, rbpp, rsb, &word);
> > + if (error)
> > + return error;
> > +
> > + /*
> > + * Modify and log the summary information.
> > + */
> > + val = xfs_suminfo_add(*rbpp, word, delta);
> > + if (mp->m_rsum_cache) {
> > + if (val == 0 && log == mp->m_rsum_cache[bbno])
> > + mp->m_rsum_cache[bbno]++;
> > + if (val != 0 && log < mp->m_rsum_cache[bbno])
> > + mp->m_rsum_cache[bbno] = log;
> > + }
> > + xfs_trans_log_rtsummary(tp, *rbpp, word);
> > + return 0;
> > +}
> > +
> > +/*
> > + * Read and return the summary information for a given extent size,
> > + * bitmap block combination.
> > + * Keeps track of a current summary block, so we don't keep reading
> > + * it from the buffer cache.
> > + */
> > +int
> > +xfs_rtget_summary(
> > + xfs_mount_t *mp, /* file system mount structure */
> > + xfs_trans_t *tp, /* transaction pointer */
> > + int log, /* log2 of extent size */
> > + xfs_fileoff_t bbno, /* bitmap block number */
> > + struct xfs_buf **rbpp, /* in/out: summary block buffer */
> > + xfs_fileoff_t *rsb, /* in/out: summary block number */
> > + xfs_suminfo_t *sum) /* out: summary info for this block */
> > +{
> > + int error;
> > + unsigned int word;
> > +
> > + error = xfs_rtmodify_summary_find(mp, tp, log, bbno, rbpp, rsb, &word);
> > + if (!error)
> > + *sum = xfs_suminfo_get(*rbpp, word);
> > + return error;
> > }
> >
> > /* Log rtbitmap block from the word @from to the byte before @next. */
> > diff --git a/fs/xfs/libxfs/xfs_rtbitmap.h b/fs/xfs/libxfs/xfs_rtbitmap.h
> > index fdfa98e0ee52f7..6d23a77def50dd 100644
> > --- a/fs/xfs/libxfs/xfs_rtbitmap.h
> > +++ b/fs/xfs/libxfs/xfs_rtbitmap.h
> > @@ -294,13 +294,12 @@ int xfs_rtfind_forw(struct xfs_mount *mp, struct xfs_trans *tp,
> > xfs_rtxnum_t *rtblock);
> > int xfs_rtmodify_range(struct xfs_mount *mp, struct xfs_trans *tp,
> > xfs_rtxnum_t start, xfs_rtxlen_t len, int val);
> > -int xfs_rtmodify_summary_int(struct xfs_mount *mp, struct xfs_trans *tp,
> > - int log, xfs_fileoff_t bbno, int delta,
> > - struct xfs_buf **rbpp, xfs_fileoff_t *rsb,
> > - xfs_suminfo_t *sum);
> > int xfs_rtmodify_summary(struct xfs_mount *mp, struct xfs_trans *tp, int log,
> > xfs_fileoff_t bbno, int delta, struct xfs_buf **rbpp,
> > xfs_fileoff_t *rsb);
> > +int xfs_rtget_summary(struct xfs_mount *mp, struct xfs_trans *tp, int log,
> > + xfs_fileoff_t bbno, struct xfs_buf **rbpp,
> > + xfs_fileoff_t *rsb, xfs_suminfo_t *sum);
> > int xfs_rtfree_range(struct xfs_mount *mp, struct xfs_trans *tp,
> > xfs_rtxnum_t start, xfs_rtxlen_t len,
> > struct xfs_buf **rbpp, xfs_fileoff_t *rsb);
> > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> > index 3be6bda2fd920c..22bc8b3b724a5b 100644
> > --- a/fs/xfs/xfs_rtalloc.c
> > +++ b/fs/xfs/xfs_rtalloc.c
> > @@ -21,25 +21,6 @@
> > #include "xfs_sb.h"
> > #include "xfs_rtbitmap.h"
> >
> > -/*
> > - * Read and return the summary information for a given extent size,
> > - * bitmap block combination.
> > - * Keeps track of a current summary block, so we don't keep reading
> > - * it from the buffer cache.
> > - */
> > -static int
> > -xfs_rtget_summary(
> > - xfs_mount_t *mp, /* file system mount structure */
> > - xfs_trans_t *tp, /* transaction pointer */
> > - int log, /* log2 of extent size */
> > - xfs_fileoff_t bbno, /* bitmap block number */
> > - struct xfs_buf **rbpp, /* in/out: summary block buffer */
> > - xfs_fileoff_t *rsb, /* in/out: summary block number */
> > - xfs_suminfo_t *sum) /* out: summary info for this block */
> > -{
> > - return xfs_rtmodify_summary_int(mp, tp, log, bbno, 0, rbpp, rsb, sum);
> > -}
> > -
> > /*
> > * Return whether there are any free extents in the size range given
> > * by low and high, for the bitmap block bbno.
