| Message ID | 20190523173742.15551-9-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/20] xfs: remove the no-op spinlock_destroy stub | expand |
On Thu, May 23, 2019 at 07:37:30PM +0200, Christoph Hellwig wrote: > Split out a self-contained chunk of code from xlog_sync that calculates > the split offset for an iclog that wraps the log end and bumps the > cycles for the second half. > > Use the chance to bring some sanity to the variables used to track the > split in xlog_sync by not changing the count variable, and instead use > split as the offset for the split and use those to calculate the > sizes and offsets for the two write buffers. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_log.c | 63 +++++++++++++++++++++++++----------------------- > 1 file changed, 33 insertions(+), 30 deletions(-) > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c > index 9a81d2d32ad9..885470f08554 100644 > --- a/fs/xfs/xfs_log.c > +++ b/fs/xfs/xfs_log.c > @@ -1804,6 +1804,32 @@ xlog_write_iclog( > xfs_buf_submit(bp); > } > > +/* > + * Bump the cycle numbers at the start of each block in the part of the iclog > + * that ends up in the buffer that gets written to the start of the log. > + * > + * Watch out for the header magic number case, though. Can we update this comment to be easier to parse? /* * We need to bump cycle number for the part of the iclog that is * written to the start of the log. Watch out for the header magic * number case, though. */ > + */ > +static unsigned int > +xlog_split_iclog( > + struct xlog *log, > + void *data, > + uint64_t bno, > + unsigned int count) > +{ > + unsigned int split_offset = BBTOB(log->l_logBBsize - bno), i; You can lose a tab from all these indents. Also, can you declare i as a separate statement - I'd prefer we don't mix multiple declarations with initialisations on the one line - it just makes the code harder to read. (i.e. I had to specifically look to find where i was declared...) > + for (i = split_offset; i < count; i += BBSIZE) { > + uint32_t cycle = get_unaligned_be32(data + i); > + > + if (++cycle == XLOG_HEADER_MAGIC_NUM) > + cycle++; > + put_unaligned_be32(cycle, data + i); Is the location we read from/write to ever unaligned? The cycle should always be 512 byte aligned to the start of the iclog data buffer, which should always be at least 4 byte aligned in memory... Otherwise the patch is fine. Cheers, Dave.
On Fri, May 24, 2019 at 09:17:26AM +1000, Dave Chinner wrote: > > +/* > > + * Bump the cycle numbers at the start of each block in the part of the iclog > > + * that ends up in the buffer that gets written to the start of the log. > > + * > > + * Watch out for the header magic number case, though. > > Can we update this comment to be easier to parse? > > /* > * We need to bump cycle number for the part of the iclog that is > * written to the start of the log. Watch out for the header magic > * number case, though. > */ Sure. > > + for (i = split_offset; i < count; i += BBSIZE) { > > + uint32_t cycle = get_unaligned_be32(data + i); > > + > > + if (++cycle == XLOG_HEADER_MAGIC_NUM) > > + cycle++; > > + put_unaligned_be32(cycle, data + i); > > Is the location we read from/write to ever unaligned? The cycle > should always be 512 byte aligned to the start of the iclog data > buffer, which should always be at least 4 byte aligned in memory... I don't think it is unaligned, but the get_unaligned_* and put_unaligned_* helpers are just really nice ways to read big/little endian fields from arbitrary char pointers, which is what we do here.
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c index 9a81d2d32ad9..885470f08554 100644 --- a/fs/xfs/xfs_log.c +++ b/fs/xfs/xfs_log.c @@ -1804,6 +1804,32 @@ xlog_write_iclog( xfs_buf_submit(bp); } +/* + * Bump the cycle numbers at the start of each block in the part of the iclog + * that ends up in the buffer that gets written to the start of the log. + * + * Watch out for the header magic number case, though. + */ +static unsigned int +xlog_split_iclog( + struct xlog *log, + void *data, + uint64_t bno, + unsigned int count) +{ + unsigned int split_offset = BBTOB(log->l_logBBsize - bno), i; + + for (i = split_offset; i < count; i += BBSIZE) { + uint32_t cycle = get_unaligned_be32(data + i); + + if (++cycle == XLOG_HEADER_MAGIC_NUM) + cycle++; + put_unaligned_be32(cycle, data + i); + } + + return split_offset; +} + /* * Flush out the in-core log (iclog) to the on-disk log in an asynchronous * fashion. Previously, we should have moved the current iclog @@ -1832,13 +1858,12 @@ xlog_sync( struct xlog *log, struct xlog_in_core *iclog) { - int i; uint count; /* byte count of bwrite */ uint count_init; /* initial count before roundup */ int roundoff; /* roundoff to BB or stripe */ - int split = 0; /* split write into two regions */ int v2 = xfs_sb_version_haslogv2(&log->l_mp->m_sb); uint64_t bno; + unsigned int split = 0; int size; bool flush = true; @@ -1881,32 +1906,8 @@ xlog_sync( bno = BLOCK_LSN(be64_to_cpu(iclog->ic_header.h_lsn)); /* Do we need to split this write into 2 parts? */ - if (bno + BTOBB(count) > log->l_logBBsize) { - char *dptr; - - split = count - (BBTOB(log->l_logBBsize - bno)); - count = BBTOB(log->l_logBBsize - bno); - iclog->ic_bwritecnt = 2; - - /* - * Bump the cycle numbers at the start of each block in the - * part of the iclog that ends up in the buffer that gets - * written to the start of the log. - * - * Watch out for the header magic number case, though. - */ - dptr = (char *)&iclog->ic_header + count; - for (i = 0; i < split; i += BBSIZE) { - uint32_t cycle = be32_to_cpu(*(__be32 *)dptr); - if (++cycle == XLOG_HEADER_MAGIC_NUM) - cycle++; - *(__be32 *)dptr = cpu_to_be32(cycle); - - dptr += BBSIZE; - } - } else { - iclog->ic_bwritecnt = 1; - } + if (bno + BTOBB(count) > log->l_logBBsize) + split = xlog_split_iclog(log, &iclog->ic_header, bno, count); /* calculcate the checksum */ iclog->ic_header.h_crc = xlog_cksum(log, &iclog->ic_header, @@ -1939,14 +1940,16 @@ xlog_sync( flush = false; } - iclog->ic_bp->b_io_length = BTOBB(count); + iclog->ic_bp->b_io_length = BTOBB(split ? split : count); + iclog->ic_bwritecnt = split ? 2 : 1; xlog_verify_iclog(log, iclog, count, true); xlog_write_iclog(log, iclog, iclog->ic_bp, bno, flush); if (split) { xfs_buf_associate_memory(iclog->ic_log->l_xbuf, - (char *)&iclog->ic_header + count, split); + (char *)&iclog->ic_header + split, + count - split); xlog_write_iclog(log, iclog, iclog->ic_log->l_xbuf, 0, false); } }
Split out a self-contained chunk of code from xlog_sync that calculates the split offset for an iclog that wraps the log end and bumps the cycles for the second half. Use the chance to bring some sanity to the variables used to track the split in xlog_sync by not changing the count variable, and instead use split as the offset for the split and use those to calculate the sizes and offsets for the two write buffers. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_log.c | 63 +++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 30 deletions(-)