[PATCH 01/18] xfs: refactor xfs_inobt_insert() to eliminate loop and support variable count

[Brian Foster]

Inodes are always allocated in chunks of 64 and thus the loop in
xfs_inobt_insert() is unnecessary. Also replace the use of hardcoded
constants (i.e., XFS_INODES_PER_CHUNK) with values provided by the
caller. This prepares the codepath to support sparse inode chunks, which
might have varying numbers of free inodes.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 37 ++++++++++++++++---------------------
1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index b62771f..6e2ccb3 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -134,37 +134,32 @@ xfs_inobt_insert(
struct xfs_mount *mp,
struct xfs_trans *tp,
struct xfs_buf *agbp,
- xfs_agino_t newino,
- xfs_agino_t newlen,
+ xfs_agino_t newino, /* start inode of record */
+ xfs_agino_t count, /* inode count */
+ xfs_inofree_t free, /* free mask */
xfs_btnum_t btnum)
{
struct xfs_btree_cur *cur;
struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
xfs_agnumber_t agno = be32_to_cpu(agi->agi_seqno);
- xfs_agino_t thisino;
int i;
int error;

cur = xfs_inobt_init_cursor(mp, tp, agbp, agno, btnum);

- for (thisino = newino;
- thisino < newino + newlen;
- thisino += XFS_INODES_PER_CHUNK) {
- error = xfs_inobt_lookup(cur, thisino, XFS_LOOKUP_EQ, &i);
- if (error) {
- xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
- return error;
- }
- ASSERT(i == 0);
+ error = xfs_inobt_lookup(cur, newino, XFS_LOOKUP_EQ, &i);
+ if (error) {
+ xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
+ return error;
+ }
+ ASSERT(i == 0);

- error = xfs_inobt_insert_rec(cur, XFS_INODES_PER_CHUNK,
- XFS_INOBT_ALL_FREE, &i);
- if (error) {
- xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
- return error;
- }
- ASSERT(i == 1);
+ error = xfs_inobt_insert_rec(cur, count, free, &i);
+ if (error) {
+ xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
+ return error;
}
+ ASSERT(i == 1);

xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);

@@ -517,13 +512,13 @@ xfs_ialloc_ag_alloc(
* Insert records describing the new inode chunk into the btrees.
*/
error = xfs_inobt_insert(args.mp, tp, agbp, newino, newlen,
- XFS_BTNUM_INO);
+ XFS_INOBT_ALL_FREE, XFS_BTNUM_INO);
if (error)
return error;

if (xfs_sb_version_hasfinobt(&args.mp->m_sb)) {
error = xfs_inobt_insert(args.mp, tp, agbp, newino, newlen,
- XFS_BTNUM_FINO);
+ XFS_INOBT_ALL_FREE, XFS_BTNUM_FINO);
if (error)
return error;
}

[Brian Foster]

Recovery of icreate transactions assumes hardcoded values for the inode
count and chunk length.

Sparse inode chunks are allocated in units of the cluster size. Update
the icreate validity checks to allow for appropriately sized inode
chunks and verify the inode count matches what is expected based on the
extent length rather than assuming a hardcoded count.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/xfs_log_recover.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index cbe782a..5994fb5 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3213,12 +3213,19 @@ xlog_recover_do_icreate_pass2(
return -EINVAL;
}

- /* existing allocation is fixed value */
- ASSERT(count == mp->m_ialloc_inos);
- ASSERT(length == mp->m_ialloc_blks);
- if (count != mp->m_ialloc_inos ||
- length != mp->m_ialloc_blks) {
- xfs_warn(log->l_mp, "xlog_recover_do_icreate_trans: bad count 2");
+ /* inode chunk is either full or sparse (cluster sized) */
+ if (length != mp->m_ialloc_blks &&
+ length != xfs_icluster_size_fsb(mp)) {
+ xfs_warn(log->l_mp,
+ "%s: unsupported chunk length", __FUNCTION__);
+ return -EINVAL;
+ }
+
+ /* verify inode count is consistent with extent length */
+ if ((count >> mp->m_sb.sb_inopblog) != length) {
+ xfs_warn(log->l_mp,
+ "%s: inconsistent inode count and chunk length",
+ __FUNCTION__);
return -EINVAL;
}

[Brian Foster]

xfs_difree_inobt() uses logic in a couple places that assume inobt
records refer to fully allocated chunks. Specifically, the use of
mp->m_ialloc_inos can cause problems for inode chunks that are sparsely
allocated. Sparse inode chunks can, by definition, define a smaller
number of inodes than a full inode chunk.

Fix the logic that determines whether an inode record should be removed
from the inobt to use the ir_free mask rather than ir_freecount.

Fix the agi counters modification to use ir_freecount to add the actual
number of inodes freed rather than assuming a full inode chunk.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 1d74394..f75f191 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1724,7 +1724,7 @@ xfs_difree_inobt(
* When an inode cluster is free, it becomes eligible for removal
*/
if (!(mp->m_flags & XFS_MOUNT_IKEEP) &&
- (rec.ir_freecount == mp->m_ialloc_inos)) {
+ (rec.ir_free == XFS_INOBT_ALL_FREE)) {

*deleted = 1;
*first_ino = XFS_AGINO_TO_INO(mp, agno, rec.ir_startino);
@@ -1734,7 +1734,7 @@ xfs_difree_inobt(
* AGI and Superblock inode counts, and mark the disk space
* to be freed when the transaction is committed.
*/
- ilen = mp->m_ialloc_inos;
+ ilen = rec.ir_freecount;
be32_add_cpu(&agi->agi_count, -ilen);
be32_add_cpu(&agi->agi_freecount, -(ilen - 1));
xfs_ialloc_log_agi(tp, agbp, XFS_AGI_COUNT | XFS_AGI_FREECOUNT);
@@ -1856,7 +1856,7 @@ xfs_difree_finobt(
* keeping inode chunks permanently on disk, remove the record.
* Otherwise, update the record with the new information.
*/
- if (rec.ir_freecount == mp->m_ialloc_inos &&
+ if (rec.ir_free == XFS_INOBT_ALL_FREE &&
!(mp->m_flags & XFS_MOUNT_IKEEP)) {
error = xfs_btree_delete(cur, &i);
if (error)

[Brian Foster]

The inobt record holemask field is a condensed data type designed to fit
into the existing on-disk record and is zero based (allocated regions
are set to 0, sparse regions are set to 1) to provide backwards
compatibility. Thus the type is unnecessarily complex for use in higher
level inode manipulations such as individual inode allocations, etc.

Rather than foist the complexity of dealing with this field to every bit
of logic that requires inode chunk allocation information, create the
xfs_inobt_ialloc_bitmap() helper to convert the inobt record holemask to
an inode allocation bitmap. The inode allocation bitmap is inode
granularity similar to the inobt record free mask and indicates which
inodes of the chunk are physically allocated on disk irrespective of
whether the inode is considered allocated or free by the filesystem.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_format.h | 1 +
fs/xfs/libxfs/xfs_ialloc.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 0baad50..cbc3296 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -206,6 +206,7 @@ typedef __be32 xfs_alloc_ptr_t;
#define XFS_FIBT_CRC_MAGIC 0x46494233 /* 'FIB3' */

typedef __uint64_t xfs_inofree_t;
+typedef __uint64_t xfs_inoalloc_t;
#define XFS_INODES_PER_CHUNK (NBBY * sizeof(xfs_inofree_t))
#define XFS_INODES_PER_CHUNK_LOG (XFS_NBBYLOG + 3)
#define XFS_INOBT_ALL_FREE ((xfs_inofree_t)-1)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 4e98a21..166602e 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -863,6 +863,73 @@ xfs_ialloc_get_rec(
}

/*
+ * Convert the inode record holemask to an inode allocation bitmap. The inode
+ * allocation bitmap is inode granularity and specifies whether an inode is
+ * physically allocated on disk (not whether the inode is considered allocated
+ * or free by the fs).
+ */
+STATIC xfs_inoalloc_t
+xfs_inobt_ialloc_bitmap(
+ struct xfs_inobt_rec_incore *rec)
+{
+ xfs_inofree_t bitmap = 0;
+ xfs_inofree_t sparsebits;
+ int nextbit;
+ int shift;
+ __uint16_t allocmask;
+ uint allocbitmap;
+
+ /*
+ * Each holemask bit represents XFS_INODES_PER_SPCHUNK inodes. Determine
+ * the inode bits per holemask bit.
+ */
+ sparsebits = xfs_mask64lo(XFS_INODES_PER_SPCHUNK);
+
+ /*
+ * The bit flip and type conversion are intentionally done separately
+ * here to zero-extend the bitmask.
+ */
+ allocmask = ~rec->ir_holemask;
+ allocbitmap = allocmask;
+
+ /*
+ * Each bit of allocbitmap represents an allocated region of the inode
+ * chunk. Thus, each bit represents XFS_INODES_PER_SPCHUNK physical
+ * inodes. Walk through allocbitmap and set the associated individual
+ * inode bits in the inode bitmap for each allocated chunk.
+ *
+ * For example, consider a 512b inode fs with a cluster size of 16k.
+ * Each holemask bit represents 4 inodes and each cluster contains 32
+ * inodes. Since sparse chunks are allocated at cluster granularity, a
+ * valid 16-bit holemask (and negated allocbitmap) with this geometry
+ * might look as follows:
+ *
+ * holemask ~ allocbitmap
+ * 0000 0000 1111 1111 => 1111 1111 0000 0000
+ *
+ * At 4 inodes per bit, this indicates that the first 32 inodes of the
+ * chunk are not physically allocated inodes. This is a hole from the
+ * perspective of the inode record. Converting the allocbitmap to a
+ * 64-bit inode allocation bitmap yields:
+ *
+ * 0xFFFFFFFF00000000
+ *
+ * ... where any of the 32 inodes defined by the higher order 32 bits of
+ * the map could be in use or free. Logically AND this bitmap with the
+ * record ir_free map to identify which of the physically allocated
+ * inodes are in use.
+ */
+ nextbit = xfs_next_bit(&allocbitmap, 1, 0);
+ while (nextbit != -1) {
+ shift = nextbit * XFS_INODES_PER_SPCHUNK;
+ bitmap |= (sparsebits << shift);
+ nextbit = xfs_next_bit(&allocbitmap, 1, nextbit + 1);
+ }
+
+ return bitmap;
+}
+
+/*
* Allocate an inode using the inobt-only algorithm.
*/
STATIC int

[Dave Chinner]

Post by Brian Foster
The inobt record holemask field is a condensed data type designed to fit
into the existing on-disk record and is zero based (allocated regions
are set to 0, sparse regions are set to 1) to provide backwards
compatibility. Thus the type is unnecessarily complex for use in higher
level inode manipulations such as individual inode allocations, etc.
Rather than foist the complexity of dealing with this field to every bit
of logic that requires inode chunk allocation information, create the
xfs_inobt_ialloc_bitmap() helper to convert the inobt record holemask to
an inode allocation bitmap. The inode allocation bitmap is inode
granularity similar to the inobt record free mask and indicates which
inodes of the chunk are physically allocated on disk irrespective of
whether the inode is considered allocated or free by the filesystem.
---
fs/xfs/libxfs/xfs_format.h | 1 +
fs/xfs/libxfs/xfs_ialloc.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 0baad50..cbc3296 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -206,6 +206,7 @@ typedef __be32 xfs_alloc_ptr_t;
#define XFS_FIBT_CRC_MAGIC 0x46494233 /* 'FIB3' */
typedef __uint64_t xfs_inofree_t;
+typedef __uint64_t xfs_inoalloc_t;
#define XFS_INODES_PER_CHUNK (NBBY * sizeof(xfs_inofree_t))
#define XFS_INODES_PER_CHUNK_LOG (XFS_NBBYLOG + 3)
#define XFS_INOBT_ALL_FREE ((xfs_inofree_t)-1)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 4e98a21..166602e 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -863,6 +863,73 @@ xfs_ialloc_get_rec(
}
/*
+ * Convert the inode record holemask to an inode allocation bitmap. The inode
+ * allocation bitmap is inode granularity and specifies whether an inode is
+ * physically allocated on disk (not whether the inode is considered allocated
+ * or free by the fs).
+ */
+STATIC xfs_inoalloc_t
+xfs_inobt_ialloc_bitmap(
+ struct xfs_inobt_rec_incore *rec)
+{
+ xfs_inofree_t bitmap = 0;
+ xfs_inofree_t sparsebits;
+ int nextbit;
+ int shift;
+ __uint16_t allocmask;
+ uint allocbitmap;

allocbitmap should be a 64 bit value?

Post by Brian Foster
+
+ /*
+ * Each holemask bit represents XFS_INODES_PER_SPCHUNK inodes. Determine
+ * the inode bits per holemask bit.
+ */
+ sparsebits = xfs_mask64lo(XFS_INODES_PER_SPCHUNK);

Can we just open code that?

sparsebits = (1ULL << XFS_INODES_PER_SPCHUNK) - 1;

Post by Brian Foster
+ /*
+ * The bit flip and type conversion are intentionally done separately
+ * here to zero-extend the bitmask.
+ */
+ allocmask = ~rec->ir_holemask;
+ allocbitmap = allocmask;

urk. That's a landmine.

Post by Brian Foster
+
+ /*
+ * Each bit of allocbitmap represents an allocated region of the inode
+ * chunk. Thus, each bit represents XFS_INODES_PER_SPCHUNK physical
+ * inodes. Walk through allocbitmap and set the associated individual
+ * inode bits in the inode bitmap for each allocated chunk.
+ *
+ * For example, consider a 512b inode fs with a cluster size of 16k.
+ * Each holemask bit represents 4 inodes and each cluster contains 32
+ * inodes. Since sparse chunks are allocated at cluster granularity, a
+ * valid 16-bit holemask (and negated allocbitmap) with this geometry
+ *
+ * holemask ~ allocbitmap
+ * 0000 0000 1111 1111 => 1111 1111 0000 0000
+ *
+ * At 4 inodes per bit, this indicates that the first 32 inodes of the
+ * chunk are not physically allocated inodes. This is a hole from the
+ * perspective of the inode record. Converting the allocbitmap to a
+ *
+ * 0xFFFFFFFF00000000
+ *
+ * ... where any of the 32 inodes defined by the higher order 32 bits of
+ * the map could be in use or free. Logically AND this bitmap with the
+ * record ir_free map to identify which of the physically allocated
+ * inodes are in use.
+ */
+ nextbit = xfs_next_bit(&allocbitmap, 1, 0);
+ while (nextbit != -1) {
+ shift = nextbit * XFS_INODES_PER_SPCHUNK;
+ bitmap |= (sparsebits << shift);
+ nextbit = xfs_next_bit(&allocbitmap, 1, nextbit + 1);
+ }

We need to get rid of xfs_next_bit() and friends, not add more
users. The generic bitmap functionality is better suited to this,
I think. A cleaned up version of something like this, perhaps:


DECLARE_BITMAP(allocmask, 16);
DECLARE_BITMAP(bitmap, 64);

bitmap_complement(&allocmask, &rec->ir_holemask, 16);
bitmap_full(&bitmap, 64);
for (i = 0; i < 16; i++) {
if (!test_bit(&allocmask, i)) {
bitmap_release_region(&bitmap, i * 4,
ilog2(XFS_INODES_PER_SPCHUNK));
}
}

Cheers,

Dave.

[Brian Foster]

Inode allocation from an existing record with free inodes traditionally
selects the first inode available according to the ir_free mask. With
sparse inode chunks, the ir_free mask could refer to an unallocated
region. We must mask the unallocated regions out of ir_free before using
it to select a free inode in the chunk.

Create the xfs_inobt_first_free_inode() helper to find the first free
inode available of the allocated regions of the inode chunk.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 166602e..1d74394 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -930,6 +930,27 @@ xfs_inobt_ialloc_bitmap(
}

/*
+ * Return the offset of the first free inode in the record. If the inode chunk
+ * is sparsely allocated, we convert the record holemask to inode granularity
+ * and mask off the unallocated regions from the inode free mask.
+ */
+STATIC int
+xfs_inobt_first_free_inode(
+ struct xfs_inobt_rec_incore *rec)
+{
+ xfs_inofree_t realfree;
+
+ /* if there are no holes, return the first available offset */
+ if (!xfs_inobt_issparse(rec))
+ return xfs_lowbit64(rec->ir_free);
+
+ realfree = xfs_inobt_ialloc_bitmap(rec);
+ realfree &= rec->ir_free;
+
+ return xfs_lowbit64(realfree);
+}
+
+/*
* Allocate an inode using the inobt-only algorithm.
*/
STATIC int
@@ -1159,7 +1180,7 @@ newino:
}

alloc_inode:
- offset = xfs_lowbit64(rec.ir_free);
+ offset = xfs_inobt_first_free_inode(&rec);
ASSERT(offset >= 0);
ASSERT(offset < XFS_INODES_PER_CHUNK);
ASSERT((XFS_AGINO_TO_OFFSET(mp, rec.ir_startino) %
@@ -1413,7 +1434,7 @@ xfs_dialloc_ag(
if (error)
goto error_cur;

- offset = xfs_lowbit64(rec.ir_free);
+ offset = xfs_inobt_first_free_inode(&rec);
ASSERT(offset >= 0);
ASSERT(offset < XFS_INODES_PER_CHUNK);
ASSERT((XFS_AGINO_TO_OFFSET(mp, rec.ir_startino) %

[Brian Foster]

Create xfs_spchunk_has_record() to receive the parameters of a new
sparse inode chunk allocation and identify whether a record exists that
is capable of tracking this sparse chunk.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 27d3437..be57b51 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -351,6 +351,61 @@ xfs_ialloc_inode_init(
}

/*
+ * Determine whether part of a sparse inode chunk that has just been allocated
+ * is covered by an existing inobt record.
+ */
+STATIC int
+xfs_spchunk_has_record(
+ struct xfs_mount *mp,
+ struct xfs_trans *tp,
+ struct xfs_buf *agbp,
+ xfs_agino_t newino,
+ xfs_agino_t count,
+ xfs_btnum_t btnum,
+ struct xfs_inobt_rec_incore *orec)
+{
+ struct xfs_btree_cur *cur;
+ struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
+ xfs_agnumber_t agno = be32_to_cpu(agi->agi_seqno);
+ xfs_agino_t previno;
+ int error;
+ int i;
+ struct xfs_inobt_rec_incore rec;
+
+ orec->ir_startino = NULLAGINO;
+
+ cur = xfs_inobt_init_cursor(mp, tp, agbp, agno, btnum);
+
+ previno = newino + count - XFS_INODES_PER_CHUNK;
+ error = xfs_inobt_lookup(cur, previno, XFS_LOOKUP_GE, &i);
+ if (error)
+ goto error;
+ if (i == 0)
+ goto out;
+
+ error = xfs_inobt_get_rec(cur, &rec, &i);
+ if (error)
+ goto error;
+ XFS_WANT_CORRUPTED_GOTO(i == 1, error);
+
+ if (rec.ir_startino > newino)
+ goto out;
+
+ ASSERT(rec.ir_startino <= newino &&
+ rec.ir_startino + XFS_INODES_PER_CHUNK > newino);
+ ASSERT(rec.ir_freecount + count <= XFS_INODES_PER_CHUNK);
+
+ *orec = rec;
+
+out:
+ xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
+ return 0;
+error:
+ xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
+ return error;
+}
+
+/*
* Allocate new inodes in the allocation group specified by agbp.
* Return 0 for success, else error code.
*/

[Dave Chinner]

Post by Brian Foster
Create xfs_spchunk_has_record() to receive the parameters of a new
sparse inode chunk allocation and identify whether a record exists that
is capable of tracking this sparse chunk.
---
fs/xfs/libxfs/xfs_ialloc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 27d3437..be57b51 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -351,6 +351,61 @@ xfs_ialloc_inode_init(
}
/*
+ * Determine whether part of a sparse inode chunk that has just been allocated
+ * is covered by an existing inobt record.
+ */
+STATIC int
+xfs_spchunk_has_record(

not sure I like the "spchunk" naming. I see that and I have no idea
what subsystem it belongs to. It's actually an inobt lookup
function, and doesn't really have anything to do with sparse chunks.
So maybe xfs_inobt_rec_exists or xfs_inobt_lookup_exact?

Post by Brian Foster
+ struct xfs_mount *mp,
+ struct xfs_trans *tp,
+ struct xfs_buf *agbp,
+ xfs_agino_t newino,
+ xfs_agino_t count,
+ xfs_btnum_t btnum,
+ struct xfs_inobt_rec_incore *orec)
+{
+ struct xfs_btree_cur *cur;
+ struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
+ xfs_agnumber_t agno = be32_to_cpu(agi->agi_seqno);
+ xfs_agino_t previno;
+ int error;
+ int i;
+ struct xfs_inobt_rec_incore rec;
+
+ orec->ir_startino = NULLAGINO;
+
+ cur = xfs_inobt_init_cursor(mp, tp, agbp, agno, btnum);
+
+ previno = newino + count - XFS_INODES_PER_CHUNK;
+ error = xfs_inobt_lookup(cur, previno, XFS_LOOKUP_GE, &i);

You want XFS_LOOKUP_EQ, yes? i.e. XFS_LOOKUP_GE won't fail if the
exact record for the inode chunk does not exist - it will return the
next one in the btree.

Post by Brian Foster
+ if (error)
+ goto error;
+ if (i == 0)
+ goto out;
+
+ error = xfs_inobt_get_rec(cur, &rec, &i);
+ if (error)
+ goto error;
+ XFS_WANT_CORRUPTED_GOTO(i == 1, error);
+
+ if (rec.ir_startino > newino)
+ goto out;

And so this check would not be necessary...

Post by Brian Foster
+
+ ASSERT(rec.ir_startino <= newino &&
+ rec.ir_startino + XFS_INODES_PER_CHUNK > newino);
+ ASSERT(rec.ir_freecount + count <= XFS_INODES_PER_CHUNK);
+
+ *orec = rec;

/* struct copy */

-Dave.

[Brian Foster]

Post by Dave Chinner

Post by Brian Foster
Create xfs_spchunk_has_record() to receive the parameters of a new
sparse inode chunk allocation and identify whether a record exists that
is capable of tracking this sparse chunk.
---
fs/xfs/libxfs/xfs_ialloc.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 27d3437..be57b51 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -351,6 +351,61 @@ xfs_ialloc_inode_init(
}
/*
+ * Determine whether part of a sparse inode chunk that has just been allocated
+ * is covered by an existing inobt record.
+ */
+STATIC int
+xfs_spchunk_has_record(

not sure I like the "spchunk" naming. I see that and I have no idea
what subsystem it belongs to. It's actually an inobt lookup
function, and doesn't really have anything to do with sparse chunks.
So maybe xfs_inobt_rec_exists or xfs_inobt_lookup_exact?

xfs_inobt_rec_exists() sounds good.

Post by Dave Chinner

Post by Brian Foster
+ struct xfs_mount *mp,
+ struct xfs_trans *tp,
+ struct xfs_buf *agbp,
+ xfs_agino_t newino,
+ xfs_agino_t count,
+ xfs_btnum_t btnum,
+ struct xfs_inobt_rec_incore *orec)
+{
+ struct xfs_btree_cur *cur;
+ struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
+ xfs_agnumber_t agno = be32_to_cpu(agi->agi_seqno);
+ xfs_agino_t previno;
+ int error;
+ int i;
+ struct xfs_inobt_rec_incore rec;
+
+ orec->ir_startino = NULLAGINO;
+
+ cur = xfs_inobt_init_cursor(mp, tp, agbp, agno, btnum);
+
+ previno = newino + count - XFS_INODES_PER_CHUNK;
+ error = xfs_inobt_lookup(cur, previno, XFS_LOOKUP_GE, &i);

You want XFS_LOOKUP_EQ, yes? i.e. XFS_LOOKUP_GE won't fail if the
exact record for the inode chunk does not exist - it will return the
next one in the btree.

Assuming variable sparse chunk granularity, I don't really know the
start ino of the record that potentially covers the new inode chunk.
Given that, we use the smallest possible start ino that could include
this chunk and search forward from there. As you've noted below, I
wasn't relying on failure here to detect the scenario where there is no
existing record.

Brian

Post by Dave Chinner

Post by Brian Foster
+ if (error)
+ goto error;
+ if (i == 0)
+ goto out;
+
+ error = xfs_inobt_get_rec(cur, &rec, &i);
+ if (error)
+ goto error;
+ XFS_WANT_CORRUPTED_GOTO(i == 1, error);
+
+ if (rec.ir_startino > newino)
+ goto out;

And so this check would not be necessary...

Post by Brian Foster
+
+ ASSERT(rec.ir_startino <= newino &&
+ rec.ir_startino + XFS_INODES_PER_CHUNK > newino);
+ ASSERT(rec.ir_freecount + count <= XFS_INODES_PER_CHUNK);
+
+ *orec = rec;

/* struct copy */
-Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Dave Chinner]

Post by Brian Foster

Post by Dave Chinner

Post by Brian Foster
+ struct xfs_btree_cur *cur;
+ struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
+ xfs_agnumber_t agno = be32_to_cpu(agi->agi_seqno);
+ xfs_agino_t previno;
+ int error;
+ int i;
+ struct xfs_inobt_rec_incore rec;
+
+ orec->ir_startino = NULLAGINO;
+
+ cur = xfs_inobt_init_cursor(mp, tp, agbp, agno, btnum);
+
+ previno = newino + count - XFS_INODES_PER_CHUNK;
+ error = xfs_inobt_lookup(cur, previno, XFS_LOOKUP_GE, &i);

You want XFS_LOOKUP_EQ, yes? i.e. XFS_LOOKUP_GE won't fail if the
exact record for the inode chunk does not exist - it will return the
next one in the btree.

Assuming variable sparse chunk granularity,

Isn't the granularity fixed for the specific filesystem
configuration as part of the on-disk format?

Post by Brian Foster
I don't really know the
start ino of the record that potentially covers the new inode chunk.
Given that, we use the smallest possible start ino that could include
this chunk and search forward from there. As you've noted below, I
wasn't relying on failure here to detect the scenario where there is no
existing record.

Ok, that's not how I thought the code was attempting to implement
the "has record" check. My mistake - a comment explaining how the
match is supposed to work would be helpful, I think.

However, with that in mind, why do you even bother calculating at
"previno"? If you want the chunk that the "newino" lies in, then
by definition it's going to be the first record at an equal or
lower start inode number than newino. i.e.:

xfs_inobt_lookup(cur, newino, XFS_LOOKUP_LE, &i);

Will return either:

- a match with startino <= newino < startino + XFS_INODES_PER_CHUNK
- a match with startino + XFS_INODES_PER_CHUNK <= newino
- a failure due to no record.

i.e. the first case is the chunk record we want, the others are
"does not exist" failures. We don't need to calculate the "previno"
at all.

Cheers,

Dave.

[Brian Foster]

Post by Dave Chinner

Post by Brian Foster

Post by Dave Chinner

Post by Brian Foster
+ struct xfs_btree_cur *cur;
+ struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
+ xfs_agnumber_t agno = be32_to_cpu(agi->agi_seqno);
+ xfs_agino_t previno;
+ int error;
+ int i;
+ struct xfs_inobt_rec_incore rec;
+
+ orec->ir_startino = NULLAGINO;
+
+ cur = xfs_inobt_init_cursor(mp, tp, agbp, agno, btnum);
+
+ previno = newino + count - XFS_INODES_PER_CHUNK;
+ error = xfs_inobt_lookup(cur, previno, XFS_LOOKUP_GE, &i);

You want XFS_LOOKUP_EQ, yes? i.e. XFS_LOOKUP_GE won't fail if the
exact record for the inode chunk does not exist - it will return the
next one in the btree.

Assuming variable sparse chunk granularity,

Isn't the granularity fixed for the specific filesystem
configuration as part of the on-disk format?

Sort of, but I'm thinking of that as a limitation of the imap code and
such. I'd like to avoid introducing more of such assumptions where
possible in the implementation. That's what I meant before about not
explicitly encoding it. I wanted to use the cluster size (now the
"spinoalignmt") only in the few places that needed the allocation size
and let the rest of the code make no assumptions and work against the
minimum granularity defined by the on-disk format (i.e., inodes per
holemask bit, inodes per record).

The only reason I had to base the sparse alloc. granularity on the
cluster size is so I don't have to go through and figure out how to fix
that inode buffer code as a dependency to get a basic mechanism working.

There's also the scenario where if the granularity can end up small
enough, I'm not sure we can reliably calculate the starting inode of a
record (unless we make changes in the allocation path). TBH, even if we
could, I'd rather keep the code flexible and warn/assert/error on the
failed assumption with more information.

Post by Dave Chinner

Post by Brian Foster
I don't really know the
start ino of the record that potentially covers the new inode chunk.
Given that, we use the smallest possible start ino that could include
this chunk and search forward from there. As you've noted below, I
wasn't relying on failure here to detect the scenario where there is no
existing record.

Ok, that's not how I thought the code was attempting to implement
the "has record" check. My mistake - a comment explaining how the
match is supposed to work would be helpful, I think.

Indeed, I'll add a comment with some context.

Post by Dave Chinner
However, with that in mind, why do you even bother calculating at
"previno"? If you want the chunk that the "newino" lies in, then
by definition it's going to be the first record at an equal or
xfs_inobt_lookup(cur, newino, XFS_LOOKUP_LE, &i);
- a match with startino <= newino < startino + XFS_INODES_PER_CHUNK
- a match with startino + XFS_INODES_PER_CHUNK <= newino
- a failure due to no record.
i.e. the first case is the chunk record we want, the others are
"does not exist" failures. We don't need to calculate the "previno"
at all.

Yeah, that might be nicer. I'll try the search in the other direction.
Thanks.

Brian

Post by Dave Chinner
Cheers,
Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Brian Foster]

xfs_ialloc_ag_select() iterates through the allocation groups looking
for free inodes or free space to determine whether to allow an inode
allocation to proceed. If no free inodes are available, it assumes that
an AG must have an extent longer than mp->m_ialloc_blks.

Sparse inode chunk support currently allows for allocations as small as
the cluster size. If sparse inodes are supported, use the cluster
alignment as a minimum extent size limit to determine whether an inode
chunk allocation attempt can proceed.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 4dd45c2..4e98a21 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -770,9 +770,15 @@ xfs_ialloc_ag_select(

/*
* Is there enough free space for the file plus a block of
- * inodes? (if we need to allocate some)?
+ * inodes? (if we need to allocate some)? If sparse inode chunks
+ * are supported, we only require an extent of length equal to
+ * the cluster size.
*/
- ineed = mp->m_ialloc_blks;
+ if (xfs_sb_version_hassparseinodes(&mp->m_sb))
+ ineed = xfs_ialloc_cluster_alignment(mp);
+ else
+ ineed = mp->m_ialloc_blks;
+
longest = pag->pagf_longest;
if (!longest)
longest = pag->pagf_flcount > 0;

[Dave Chinner]

Post by Brian Foster
xfs_ialloc_ag_select() iterates through the allocation groups looking
for free inodes or free space to determine whether to allow an inode
allocation to proceed. If no free inodes are available, it assumes that
an AG must have an extent longer than mp->m_ialloc_blks.
Sparse inode chunk support currently allows for allocations as small as
the cluster size. If sparse inodes are supported, use the cluster
alignment as a minimum extent size limit to determine whether an inode
chunk allocation attempt can proceed.
---
fs/xfs/libxfs/xfs_ialloc.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 4dd45c2..4e98a21 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -770,9 +770,15 @@ xfs_ialloc_ag_select(
/*
* Is there enough free space for the file plus a block of
- * inodes? (if we need to allocate some)?
+ * inodes? (if we need to allocate some)? If sparse inode chunks
+ * are supported, we only require an extent of length equal to
+ * the cluster size.
*/
- ineed = mp->m_ialloc_blks;
+ if (xfs_sb_version_hassparseinodes(&mp->m_sb))
+ ineed = xfs_ialloc_cluster_alignment(mp);
+ else
+ ineed = mp->m_ialloc_blks;

Why wouldn't we calculate this once at mount time and dump it in
mp->m_ialloc_min_blks? And if we don't have sparse inodes enabled,
when would be ever select an AG that would trigger a sparse
allocation and then turn the feature bit on?

Cheers,

Dave.

[Brian Foster]

The sparse inode chunks feature will use the helper function to enable
the allocation of sparse inode chunks. The incompatible feature bit is
set on disk once a sparse inode chunk is allocated to prevent older
drivers from mounting an fs with sparse chunks.

Note that the feature is hardcoded disabled and the feature bit not
included in the all features mask until fully implemented.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_sb.h | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
index c43c2d6..6f48de9 100644
--- a/fs/xfs/libxfs/xfs_sb.h
+++ b/fs/xfs/libxfs/xfs_sb.h
@@ -509,6 +509,7 @@ xfs_sb_has_ro_compat_feature(
}

#define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */
+#define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */
#define XFS_SB_FEAT_INCOMPAT_ALL \
(XFS_SB_FEAT_INCOMPAT_FTYPE)

@@ -558,6 +559,11 @@ static inline int xfs_sb_version_hasfinobt(xfs_sb_t *sbp)
(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_FINOBT);
}

+static inline int xfs_sb_version_hassparseinodes(struct xfs_sb *sbp)
+{
+ return 0; /* not yet enabled */
+}
+
/*
* end of superblock version macros
*/

[Mark Tinguely]

Post by Brian Foster
The sparse inode chunks feature will use the helper function to enable
the allocation of sparse inode chunks. The incompatible feature bit is
set on disk once a sparse inode chunk is allocated to prevent older
drivers from mounting an fs with sparse chunks.
Note that the feature is hardcoded disabled and the feature bit not
included in the all features mask until fully implemented.
---

Sorry if I missed it in the series but is there an
XFS_FSOP_GEOM_FLAGS_SPINODES for xfs_info/growfs?

Thanks,

--Mark.

[Brian Foster]

Post by Mark Tinguely

Post by Brian Foster
The sparse inode chunks feature will use the helper function to enable
the allocation of sparse inode chunks. The incompatible feature bit is
set on disk once a sparse inode chunk is allocated to prevent older
drivers from mounting an fs with sparse chunks.
Note that the feature is hardcoded disabled and the feature bit not
included in the all features mask until fully implemented.
---

Sorry if I missed it in the series but is there an
XFS_FSOP_GEOM_FLAGS_SPINODES for xfs_info/growfs?

Nope, looks like I missed it. It probably slipped my mind as I haven't
got into userspace yet and thus hadn't thought about xfs_info. I'll make
a note to add it, thanks!

Brian

Post by Mark Tinguely
Thanks,
--Mark.

[Mark Tinguely]

Post by Brian Foster

Post by Mark Tinguely

Post by Brian Foster
The sparse inode chunks feature will use the helper function to enable
the allocation of sparse inode chunks. The incompatible feature bit is
set on disk once a sparse inode chunk is allocated to prevent older
drivers from mounting an fs with sparse chunks.
Note that the feature is hardcoded disabled and the feature bit not
included in the all features mask until fully implemented.
---

Sorry if I missed it in the series but is there an
XFS_FSOP_GEOM_FLAGS_SPINODES for xfs_info/growfs?

Nope, looks like I missed it. It probably slipped my mind as I haven't
got into userspace yet and thus hadn't thought about xfs_info. I'll make
a note to add it, thanks!
Brian

Again forgive my quick scanning of the series, but am I correct in
thinking that this does not change the minimum number of reserved blocks
for create like fs ops. The create/rename do some attempts to continue
when it cannot get the full number of reserved blocks. Would allocating
a sparse inode chunk make sense in that case? My gut says the
complication does not.

Thanks again,

--Mark.

[Brian Foster]

Post by Brian Foster

Post by Mark Tinguely

Post by Brian Foster
The sparse inode chunks feature will use the helper function to enable
the allocation of sparse inode chunks. The incompatible feature bit is
set on disk once a sparse inode chunk is allocated to prevent older
drivers from mounting an fs with sparse chunks.
Note that the feature is hardcoded disabled and the feature bit not
included in the all features mask until fully implemented.
---

Sorry if I missed it in the series but is there an
XFS_FSOP_GEOM_FLAGS_SPINODES for xfs_info/growfs?

Nope, looks like I missed it. It probably slipped my mind as I haven't
got into userspace yet and thus hadn't thought about xfs_info. I'll make
a note to add it, thanks!
Brian

Again forgive my quick scanning of the series, but am I correct in thinking
that this does not change the minimum number of reserved blocks for create
like fs ops. The create/rename do some attempts to continue when it cannot
get the full number of reserved blocks. Would allocating a sparse inode
chunk make sense in that case? My gut says the complication does not.

No problem, thanks for looking.

Correct... I haven't made any changes to transaction reservations or
things like XFS_CREATE_SPACE_RES(), etc. xfs_create() has the kind of
logic you describe where we set resblks = 0 if the reservation fails
with ENOSPC and try again with a flag set to indicate we can't do
allocations. My understanding is that this is justified since we could
always allocate an inode from an existing record and a dentry from an
unused slot in a directory.

Technically, I think we could explicitly allocate a sparse chunk in this
scenario if we wanted to, but we'd have to pass that down to/through the
allocation code in xfs_ialloc_ag_alloc(). I could be wrong, but I think
the reservation failure in this case will always be limited to a case
where we are truly up against ENOSPC (as opposed to failing due to an
extent length or alignment requirement), since the reservation comes out
of the sb counters. Given that, I tend to agree that it might not be
worth the extra complexity there.

Brian

Thanks again,
--Mark.

[Dave Chinner]

Post by Brian Foster
The sparse inode chunks feature will use the helper function to enable
the allocation of sparse inode chunks. The incompatible feature bit is
set on disk once a sparse inode chunk is allocated to prevent older
drivers from mounting an fs with sparse chunks.
Note that the feature is hardcoded disabled and the feature bit not
included in the all features mask until fully implemented.
---
fs/xfs/libxfs/xfs_sb.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
index c43c2d6..6f48de9 100644
--- a/fs/xfs/libxfs/xfs_sb.h
+++ b/fs/xfs/libxfs/xfs_sb.h
@@ -509,6 +509,7 @@ xfs_sb_has_ro_compat_feature(
}
#define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */
+#define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */
#define XFS_SB_FEAT_INCOMPAT_ALL \
(XFS_SB_FEAT_INCOMPAT_FTYPE)
@@ -558,6 +559,11 @@ static inline int xfs_sb_version_hasfinobt(xfs_sb_t *sbp)
(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_FINOBT);
}
+static inline int xfs_sb_version_hassparseinodes(struct xfs_sb *sbp)

static inline bool ...

Post by Brian Foster
+{
+ return 0; /* not yet enabled */

return false;

-Dave.

[Brian Foster]

An inode chunk is currently added to the transaction free list based on
a simple fsb conversion and hardcoded chunk length. The nature of sparse
chunks is such that the physical chunk of inodes on disk may consist of
one or more discontiguous parts. Blocks that reside in the holes of the
inode chunk are not inodes and could be allocated to any other use or
not allocated at all.

Refactor the existing xfs_bmap_add_free() call into the
xfs_difree_inode_chunk() helper. The new helper uses the existing
calculation if a chunk is not sparse. Otherwise, use the inobt record
holemask to free the contiguous regions of the chunk.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 61 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index f75f191..1be57b1 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1659,6 +1659,66 @@ out_error:
return error;
}

+/*
+ * Free the blocks of an inode chunk. We must consider that the inode chunk
+ * might be sparse and only free the regions that are allocated as part of the
+ * chunk.
+ */
+STATIC void
+xfs_difree_inode_chunk(
+ struct xfs_mount *mp,
+ xfs_agnumber_t agno,
+ struct xfs_inobt_rec_incore *rec,
+ struct xfs_bmap_free *flist)
+{
+ xfs_agblock_t sagbno = XFS_AGINO_TO_AGBNO(mp, rec->ir_startino);
+ xfs_agblock_t agbno;
+ int nextbit;
+ int contig, contigblk;
+ __uint16_t allocmask;
+ uint allocbitmap;
+
+ if (!xfs_inobt_issparse(rec)) {
+ /* not sparse, calculate extent info directly */
+ xfs_bmap_add_free(XFS_AGB_TO_FSB(mp, agno,
+ XFS_AGINO_TO_AGBNO(mp, rec->ir_startino)),
+ mp->m_ialloc_blks, flist, mp);
+ return;
+ }
+
+ /*
+ * The bit flip and type conversion are intentionally done separately
+ * here to zero-extend the bitmask.
+ */
+ allocmask = ~rec->ir_holemask;
+ allocbitmap = allocmask;
+
+ /*
+ * We now have an allocation bitmap in units of inodes at sparse chunk
+ * granularity (e.g., more than one inode per bit). Use the bitmask
+ * functions to find each contigious range of bits in the map. For each
+ * range, convert the start bit and count to block values and use that
+ * data to add the associated extent to the free list.
+ */
+ nextbit = xfs_next_bit(&allocbitmap, 1, 0);
+ while (nextbit != -1) {
+ agbno = (nextbit * XFS_INODES_PER_SPCHUNK) /
+ mp->m_sb.sb_inopblock;
+ agbno += sagbno;
+
+ contig = xfs_contig_bits(&allocbitmap, 1, nextbit);
+ contigblk = (contig * XFS_INODES_PER_SPCHUNK) /
+ mp->m_sb.sb_inopblock;
+
+ ASSERT(agbno % xfs_ialloc_cluster_alignment(mp) == 0);
+ ASSERT(contigblk % xfs_ialloc_cluster_alignment(mp) == 0);
+ xfs_bmap_add_free(XFS_AGB_TO_FSB(mp, agno, agbno), contigblk,
+ flist, mp);
+
+ nextbit = xfs_next_bit(&allocbitmap, 1, nextbit + contig + 1);
+ }
+}
+
STATIC int
xfs_difree_inobt(
struct xfs_mount *mp,
@@ -1750,9 +1810,7 @@ xfs_difree_inobt(
goto error0;
}

- xfs_bmap_add_free(XFS_AGB_TO_FSB(mp, agno,
- XFS_AGINO_TO_AGBNO(mp, rec.ir_startino)),
- mp->m_ialloc_blks, flist, mp);
+ xfs_difree_inode_chunk(mp, agno, &rec, flist);
} else {
*deleted = 0;

[Dave Chinner]

Post by Brian Foster
An inode chunk is currently added to the transaction free list based on
a simple fsb conversion and hardcoded chunk length. The nature of sparse
chunks is such that the physical chunk of inodes on disk may consist of
one or more discontiguous parts. Blocks that reside in the holes of the
inode chunk are not inodes and could be allocated to any other use or
not allocated at all.
Refactor the existing xfs_bmap_add_free() call into the
xfs_difree_inode_chunk() helper. The new helper uses the existing
calculation if a chunk is not sparse. Otherwise, use the inobt record
holemask to free the contiguous regions of the chunk.
---
fs/xfs/libxfs/xfs_ialloc.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 61 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index f75f191..1be57b1 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
return error;
}
+/*
+ * Free the blocks of an inode chunk. We must consider that the inode chunk
+ * might be sparse and only free the regions that are allocated as part of the
+ * chunk.
+ */
+STATIC void
+xfs_difree_inode_chunk(
+ struct xfs_mount *mp,
+ xfs_agnumber_t agno,
+ struct xfs_inobt_rec_incore *rec,
+ struct xfs_bmap_free *flist)
+{
+ xfs_agblock_t sagbno = XFS_AGINO_TO_AGBNO(mp, rec->ir_startino);
+ xfs_agblock_t agbno;
+ int nextbit;
+ int contig, contigblk;
+ __uint16_t allocmask;
+ uint allocbitmap;
+
+ if (!xfs_inobt_issparse(rec)) {
+ /* not sparse, calculate extent info directly */
+ xfs_bmap_add_free(XFS_AGB_TO_FSB(mp, agno,
+ XFS_AGINO_TO_AGBNO(mp, rec->ir_startino)),
+ mp->m_ialloc_blks, flist, mp);
+ return;
+ }
+
+ /*
+ * The bit flip and type conversion are intentionally done separately
+ * here to zero-extend the bitmask.
+ */
+ allocmask = ~rec->ir_holemask;
+ allocbitmap = allocmask;

If we are going to keep this code, then a helper is absolutely
necessary....

Post by Brian Foster
+
+ /*
+ * We now have an allocation bitmap in units of inodes at sparse chunk
+ * granularity (e.g., more than one inode per bit). Use the bitmask
+ * functions to find each contigious range of bits in the map. For each
+ * range, convert the start bit and count to block values and use that
+ * data to add the associated extent to the free list.
+ */
+ nextbit = xfs_next_bit(&allocbitmap, 1, 0);
+ while (nextbit != -1) {
+ agbno = (nextbit * XFS_INODES_PER_SPCHUNK) /
+ mp->m_sb.sb_inopblock;
+ agbno += sagbno;
+
+ contig = xfs_contig_bits(&allocbitmap, 1, nextbit);
+ contigblk = (contig * XFS_INODES_PER_SPCHUNK) /
+ mp->m_sb.sb_inopblock;
+
+ ASSERT(agbno % xfs_ialloc_cluster_alignment(mp) == 0);
+ ASSERT(contigblk % xfs_ialloc_cluster_alignment(mp) == 0);
+ xfs_bmap_add_free(XFS_AGB_TO_FSB(mp, agno, agbno), contigblk,
+ flist, mp);
+
+ nextbit = xfs_next_bit(&allocbitmap, 1, nextbit + contig + 1);
+ }

Again, I think that the generic bitmap code is a better way to
implement this...

Cheers,

Dave.

[Brian Foster]

xfs_ialloc_cluster_alignment() takes an args parameter but only uses the
xfs_mount pointer. Replace the args parameter with mp such that we can
use this function from additional contexts.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 6e2ccb3..5448a74 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -48,12 +48,12 @@
*/
static inline int
xfs_ialloc_cluster_alignment(
- xfs_alloc_arg_t *args)
+ struct xfs_mount *mp)
{
- if (xfs_sb_version_hasalign(&args->mp->m_sb) &&
- args->mp->m_sb.sb_inoalignmt >=
- XFS_B_TO_FSBT(args->mp, args->mp->m_inode_cluster_size))
- return args->mp->m_sb.sb_inoalignmt;
+ if (xfs_sb_version_hasalign(&mp->m_sb) &&
+ mp->m_sb.sb_inoalignmt >=
+ XFS_B_TO_FSBT(mp, mp->m_inode_cluster_size))
+ return mp->m_sb.sb_inoalignmt;
return 1;
}

@@ -407,7 +407,7 @@ xfs_ialloc_ag_alloc(
* but not to use them in the actual exact allocation.
*/
args.alignment = 1;
- args.minalignslop = xfs_ialloc_cluster_alignment(&args) - 1;
+ args.minalignslop = xfs_ialloc_cluster_alignment(args.mp) - 1;

/* Allow space for the inode btree to split. */
args.minleft = args.mp->m_in_maxlevels - 1;
@@ -443,7 +443,7 @@ xfs_ialloc_ag_alloc(
args.alignment = args.mp->m_dalign;
isaligned = 1;
} else
- args.alignment = xfs_ialloc_cluster_alignment(&args);
+ args.alignment = xfs_ialloc_cluster_alignment(args.mp);
/*
* Need to figure out where to allocate the inode blocks.
* Ideally they should be spaced out through the a.g.
@@ -472,7 +472,7 @@ xfs_ialloc_ag_alloc(
args.type = XFS_ALLOCTYPE_NEAR_BNO;
args.agbno = be32_to_cpu(agi->agi_root);
args.fsbno = XFS_AGB_TO_FSB(args.mp, agno, args.agbno);
- args.alignment = xfs_ialloc_cluster_alignment(&args);
+ args.alignment = xfs_ialloc_cluster_alignment(args.mp);
if ((error = xfs_alloc_vextent(&args)))
return error;
}

[Brian Foster]

xfs_ifree_cluster() is called to mark all in-memory inodes and inode
buffers as stale. This occurs after we've removed the inobt records and
dropped any references of inobt data. xfs_ifree_cluster() uses the
starting inode number to walk the namespace of inodes expected for a
single chunk a cluster buffer at a time. The cluster buffer disk
addresses are calculated by decoding the sequential inode numbers
expected from the chunk.

The problem with this approach is that if the inode chunk being removed
is a sparse chunk, not all of the buffer addresses that are calculated
as part of this sequence may be inode clusters. Attempting to acquire
the buffer based on expected inode characterstics (i.e., cluster length)
can lead to errors and is generally incorrect.

We already use a couple variables to carry requisite state from
xfs_difree() to xfs_ifree_cluster(). Rather than add a third, define a
new internal structure to carry the existing parameters through these
functions. Add an alloc field that represents the physical allocation
bitmap of inodes in the chunk being removed. Modify xfs_ifree_cluster()
to check each inode against the bitmap and skip the clusters that were
never allocated as real inodes on disk.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 16 +++++++---------
fs/xfs/libxfs/xfs_ialloc.h | 10 ++++++++--
fs/xfs/xfs_inode.c | 28 ++++++++++++++++++++--------
3 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 1be57b1..86c6ccd 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -1726,8 +1726,7 @@ xfs_difree_inobt(
struct xfs_buf *agbp,
xfs_agino_t agino,
struct xfs_bmap_free *flist,
- int *deleted,
- xfs_ino_t *first_ino,
+ struct xfs_icluster *xic,
struct xfs_inobt_rec_incore *orec)
{
struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
@@ -1786,8 +1785,9 @@ xfs_difree_inobt(
if (!(mp->m_flags & XFS_MOUNT_IKEEP) &&
(rec.ir_free == XFS_INOBT_ALL_FREE)) {

- *deleted = 1;
- *first_ino = XFS_AGINO_TO_INO(mp, agno, rec.ir_startino);
+ xic->deleted = 1;
+ xic->first_ino = XFS_AGINO_TO_INO(mp, agno, rec.ir_startino);
+ xic->alloc = xfs_inobt_ialloc_bitmap(&rec);

/*
* Remove the inode cluster from the AGI B+Tree, adjust the
@@ -1812,7 +1812,7 @@ xfs_difree_inobt(

xfs_difree_inode_chunk(mp, agno, &rec, flist);
} else {
- *deleted = 0;
+ xic->deleted = 0;

error = xfs_inobt_update(cur, &rec);
if (error) {
@@ -1950,8 +1950,7 @@ xfs_difree(
struct xfs_trans *tp, /* transaction pointer */
xfs_ino_t inode, /* inode to be freed */
struct xfs_bmap_free *flist, /* extents to free */
- int *deleted,/* set if inode cluster was deleted */
- xfs_ino_t *first_ino)/* first inode in deleted cluster */
+ struct xfs_icluster *xic) /* cluster info if deleted */
{
/* REFERENCED */
xfs_agblock_t agbno; /* block number containing inode */
@@ -2002,8 +2001,7 @@ xfs_difree(
/*
* Fix up the inode allocation btree.
*/
- error = xfs_difree_inobt(mp, tp, agbp, agino, flist, deleted, first_ino,
- &rec);
+ error = xfs_difree_inobt(mp, tp, agbp, agino, flist, xic, &rec);
if (error)
goto error0;

diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 62c1381..5aa8d6f 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -28,6 +28,13 @@ struct xfs_btree_cur;
/* Move inodes in clusters of this size */
#define XFS_INODE_BIG_CLUSTER_SIZE 8192

+struct xfs_icluster {
+ bool deleted; /* record is deleted */
+ xfs_ino_t first_ino; /* first inode number */
+ xfs_inoalloc_t alloc; /* inode phys. allocation bitmap for
+ * sparse chunks */
+};
+
/* Calculate and return the number of filesystem blocks per inode cluster */
static inline int
xfs_icluster_size_fsb(
@@ -90,8 +97,7 @@ xfs_difree(
struct xfs_trans *tp, /* transaction pointer */
xfs_ino_t inode, /* inode to be freed */
struct xfs_bmap_free *flist, /* extents to free */
- int *deleted, /* set if inode cluster was deleted */
- xfs_ino_t *first_ino); /* first inode in deleted cluster */
+ struct xfs_icluster *ifree); /* cluster info if deleted */

/*
* Return the location of the inode in imap, for mapping it into a buffer.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 1a5e068..641fe34 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2181,9 +2181,9 @@ xfs_iunlink_remove(
*/
STATIC int
xfs_ifree_cluster(
- xfs_inode_t *free_ip,
- xfs_trans_t *tp,
- xfs_ino_t inum)
+ xfs_inode_t *free_ip,
+ xfs_trans_t *tp,
+ struct xfs_icluster *xic)
{
xfs_mount_t *mp = free_ip->i_mount;
int blks_per_cluster;
@@ -2196,13 +2196,26 @@ xfs_ifree_cluster(
xfs_inode_log_item_t *iip;
xfs_log_item_t *lip;
struct xfs_perag *pag;
+ xfs_ino_t inum;

+ inum = xic->first_ino;
pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, inum));
blks_per_cluster = xfs_icluster_size_fsb(mp);
inodes_per_cluster = blks_per_cluster << mp->m_sb.sb_inopblog;
nbufs = mp->m_ialloc_blks / blks_per_cluster;

for (j = 0; j < nbufs; j++, inum += inodes_per_cluster) {
+ /*
+ * The allocation bitmap tells us which inodes of the chunk were
+ * physically allocated. Skip the cluster if an inode falls into
+ * a sparse region.
+ */
+ if ((xic->alloc & XFS_INOBT_MASK(inum - xic->first_ino)) == 0) {
+ ASSERT(((inum - xic->first_ino) %
+ inodes_per_cluster) == 0);
+ continue;
+ }
+
blkno = XFS_AGB_TO_DADDR(mp, XFS_INO_TO_AGNO(mp, inum),
XFS_INO_TO_AGBNO(mp, inum));

@@ -2360,8 +2373,7 @@ xfs_ifree(
xfs_bmap_free_t *flist)
{
int error;
- int delete;
- xfs_ino_t first_ino;
+ struct xfs_icluster xic = { 0 };

ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
ASSERT(ip->i_d.di_nlink == 0);
@@ -2377,7 +2389,7 @@ xfs_ifree(
if (error)
return error;

- error = xfs_difree(tp, ip->i_ino, flist, &delete, &first_ino);
+ error = xfs_difree(tp, ip->i_ino, flist, &xic);
if (error)
return error;

@@ -2394,8 +2406,8 @@ xfs_ifree(
ip->i_d.di_gen++;
xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);

- if (delete)
- error = xfs_ifree_cluster(ip, tp, first_ino);
+ if (xic.deleted)
+ error = xfs_ifree_cluster(ip, tp, &xic);

return error;
}

[Brian Foster]

The sparse chunk inode record format is backwards compatible with old
format inobt records as long as full chunks are allocated. The holemask
field uses higher order bytes of the freecount. While sparse chunks can
be enabled on previously unsupported fs, older kernel drivers cannot
parse sparse inode records.

Set the feature incompatible bit once a sparse inode chunk is allocated
to prevent older XFS drivers from tripping over the new format.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 4226b1b..4dd45c2 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -407,6 +407,27 @@ error:
return error;
}

+STATIC void
+xfs_sbversion_add_spinodes(
+ struct xfs_trans *tp,
+ struct xfs_mount *mp)
+{
+ if (xfs_sb_has_incompat_log_feature(&mp->m_sb,
+ XFS_SB_FEAT_INCOMPAT_SPINODES))
+ return;
+
+ spin_lock(&mp->m_sb_lock);
+ if (xfs_sb_has_incompat_log_feature(&mp->m_sb,
+ XFS_SB_FEAT_INCOMPAT_SPINODES)) {
+ spin_unlock(&mp->m_sb_lock);
+ return;
+ }
+
+ mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_SPINODES;
+ spin_unlock(&mp->m_sb_lock);
+ xfs_mod_sb(tp, XFS_SB_FEATURES_INCOMPAT);
+}
+
/*
* Allocate new inodes in the allocation group specified by agbp.
* Return 0 for success, else error code.
@@ -631,6 +652,18 @@ xfs_ialloc_ag_alloc(
if (error)
return error;
}
+
+ /*
+ * Set an incompat feature bit as old drivers can't parse sparse
+ * records. Pre-sparse inode chunk drivers will include the
+ * holemask in the higher order freecount bits, resulting in a
+ * bogus value.
+ *
+ * XXX: when is this bit removed?
+ */
+ if (xfs_inobt_issparse(&rec))
+ xfs_sbversion_add_spinodes(tp, args.mp);
+
/*
* Log allocation group header fields
*/

[Dave Chinner]

Post by Brian Foster
The sparse chunk inode record format is backwards compatible with old
format inobt records as long as full chunks are allocated. The holemask
field uses higher order bytes of the freecount. While sparse chunks can
be enabled on previously unsupported fs, older kernel drivers cannot
parse sparse inode records.
Set the feature incompatible bit once a sparse inode chunk is allocated
to prevent older XFS drivers from tripping over the new format.
---
fs/xfs/libxfs/xfs_ialloc.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 4226b1b..4dd45c2 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
return error;
}
+STATIC void
+xfs_sbversion_add_spinodes(
+ struct xfs_trans *tp,
+ struct xfs_mount *mp)
+{
+ if (xfs_sb_has_incompat_log_feature(&mp->m_sb,
+ XFS_SB_FEAT_INCOMPAT_SPINODES))

wrong function. xfs_sb_has_incompat_feature().

Post by Brian Foster
+ return;
+
+ spin_lock(&mp->m_sb_lock);
+ if (xfs_sb_has_incompat_log_feature(&mp->m_sb,
+ XFS_SB_FEAT_INCOMPAT_SPINODES)) {
+ spin_unlock(&mp->m_sb_lock);
+ return;
+ }
+
+ mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_SPINODES;
+ spin_unlock(&mp->m_sb_lock);
+ xfs_mod_sb(tp, XFS_SB_FEATURES_INCOMPAT);
+}
+
/*
* Allocate new inodes in the allocation group specified by agbp.
* Return 0 for success, else error code.
@@ -631,6 +652,18 @@ xfs_ialloc_ag_alloc(
if (error)
return error;
}
+
+ /*
+ * Set an incompat feature bit as old drivers can't parse sparse
+ * records. Pre-sparse inode chunk drivers will include the
+ * holemask in the higher order freecount bits, resulting in a
+ * bogus value.
+ *
+ * XXX: when is this bit removed?
+ */
+ if (xfs_inobt_issparse(&rec))
+ xfs_sbversion_add_spinodes(tp, args.mp);

I'd like to avoid this dynamic feature bit adding if possible. Do we
really need it?

Cheers,

Dave.

[Brian Foster]

Post by Dave Chinner

Post by Brian Foster
The sparse chunk inode record format is backwards compatible with old
format inobt records as long as full chunks are allocated. The holemask
field uses higher order bytes of the freecount. While sparse chunks can
be enabled on previously unsupported fs, older kernel drivers cannot
parse sparse inode records.
Set the feature incompatible bit once a sparse inode chunk is allocated
to prevent older XFS drivers from tripping over the new format.
---
fs/xfs/libxfs/xfs_ialloc.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 4226b1b..4dd45c2 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
return error;
}
+STATIC void
+xfs_sbversion_add_spinodes(
+ struct xfs_trans *tp,
+ struct xfs_mount *mp)
+{
+ if (xfs_sb_has_incompat_log_feature(&mp->m_sb,
+ XFS_SB_FEAT_INCOMPAT_SPINODES))

wrong function. xfs_sb_has_incompat_feature().

Oops, right.

Post by Dave Chinner

Post by Brian Foster
+ return;
+
+ spin_lock(&mp->m_sb_lock);
+ if (xfs_sb_has_incompat_log_feature(&mp->m_sb,
+ XFS_SB_FEAT_INCOMPAT_SPINODES)) {
+ spin_unlock(&mp->m_sb_lock);
+ return;
+ }
+
+ mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_SPINODES;
+ spin_unlock(&mp->m_sb_lock);
+ xfs_mod_sb(tp, XFS_SB_FEATURES_INCOMPAT);
+}
+
/*
* Allocate new inodes in the allocation group specified by agbp.
* Return 0 for success, else error code.
@@ -631,6 +652,18 @@ xfs_ialloc_ag_alloc(
if (error)
return error;
}
+
+ /*
+ * Set an incompat feature bit as old drivers can't parse sparse
+ * records. Pre-sparse inode chunk drivers will include the
+ * holemask in the higher order freecount bits, resulting in a
+ * bogus value.
+ *
+ * XXX: when is this bit removed?
+ */
+ if (xfs_inobt_issparse(&rec))
+ xfs_sbversion_add_spinodes(tp, args.mp);

I'd like to avoid this dynamic feature bit adding if possible. Do we
really need it?

Nope. As discussed, I'll move to something purely manual for the next
iteration and we can move forward from there with regard to if/when to
set it by default.

Brian

Post by Dave Chinner
Cheers,
Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Brian Foster]

xfs_ialloc_ag_alloc() makes several attempts to allocate a full inode
chunk. If all else fails, reduce the allocation to inode cluster size
and attempt to allocate a sparse inode chunk.

If sparse chunk allocation succeeds, check whether an inobt record
already exists that can track the chunk. If so, inherit and update the
existing record. Otherwise, insert a new record for the sparse chunk.

Update xfs_inobt_insert_rec() to take the holemask as a parameter and
set the associated field on disk. Convert xfs_inobt_insert() to
xfs_inobt_update_insert() to handle record insertion or update in a
generic fashion. This facilitates the continued use of the same function
for the inobt and finobt.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 105 +++++++++++++++++++++++++++++++++------------
1 file changed, 77 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index be57b51..4226b1b 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -121,28 +121,28 @@ xfs_inobt_get_rec(
STATIC int
xfs_inobt_insert_rec(
struct xfs_btree_cur *cur,
+ __uint16_t holemask,
__int32_t freecount,
xfs_inofree_t free,
int *stat)
{
- cur->bc_rec.i.ir_holemask = 0;
+ cur->bc_rec.i.ir_holemask = holemask;
cur->bc_rec.i.ir_freecount = freecount;
cur->bc_rec.i.ir_free = free;
return xfs_btree_insert(cur, stat);
}

/*
- * Insert records describing a newly allocated inode chunk into the inobt.
+ * Update or insert records describing a newly allocated inode chunk into the
+ * specified inobt.
*/
STATIC int
-xfs_inobt_insert(
- struct xfs_mount *mp,
- struct xfs_trans *tp,
- struct xfs_buf *agbp,
- xfs_agino_t newino, /* start inode of record */
- xfs_agino_t count, /* inode count */
- xfs_inofree_t free, /* free mask */
- xfs_btnum_t btnum)
+xfs_inobt_update_insert(
+ struct xfs_mount *mp,
+ struct xfs_trans *tp,
+ struct xfs_buf *agbp,
+ struct xfs_inobt_rec_incore *rec, /* record to update/insert */
+ xfs_btnum_t btnum)
{
struct xfs_btree_cur *cur;
struct xfs_agi *agi = XFS_BUF_TO_AGI(agbp);
@@ -152,23 +152,25 @@ xfs_inobt_insert(

cur = xfs_inobt_init_cursor(mp, tp, agbp, agno, btnum);

- error = xfs_inobt_lookup(cur, newino, XFS_LOOKUP_EQ, &i);
- if (error) {
- xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
- return error;
- }
- ASSERT(i == 0);
-
- error = xfs_inobt_insert_rec(cur, count, free, &i);
- if (error) {
- xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
- return error;
+ error = xfs_inobt_lookup(cur, rec->ir_startino, XFS_LOOKUP_EQ, &i);
+ if (i == 1) {
+ error = xfs_inobt_update(cur, rec);
+ if (error)
+ goto error;
+ } else {
+ error = xfs_inobt_insert_rec(cur, rec->ir_holemask,
+ rec->ir_freecount, rec->ir_free, &i);
+ if (error)
+ goto error;
+ ASSERT(i == 1);
}
- ASSERT(i == 1);

xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
-
return 0;
+
+error:
+ xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
+ return error;
}

/*
@@ -423,6 +425,10 @@ xfs_ialloc_ag_alloc(
xfs_agino_t newlen; /* new number of inodes */
int isaligned = 0; /* inode allocation at stripe unit */
/* boundary */
+ uint16_t allocmask = (uint16_t) -1; /* init. to full chunk */
+ struct xfs_inobt_rec_incore rec;
+ int offset;
+
struct xfs_perag *pag;

memset(&args, 0, sizeof(args));
@@ -538,6 +544,28 @@ xfs_ialloc_ag_alloc(
return error;
}

+ /*
+ * Finally, try a sparse allocation if the filesystem supports it.
+ */
+ if (xfs_sb_version_hassparseinodes(&args.mp->m_sb) &&
+ args.fsbno == NULLFSBLOCK) {
+ args.type = XFS_ALLOCTYPE_NEAR_BNO;
+ args.agbno = be32_to_cpu(agi->agi_root);
+ args.fsbno = XFS_AGB_TO_FSB(args.mp, agno, args.agbno);
+ args.alignment = xfs_ialloc_cluster_alignment(args.mp);
+
+ /* allocate sparse regions in cluster granularity */
+ args.minlen = xfs_ialloc_cluster_alignment(args.mp);
+ args.maxlen = args.minlen;
+
+ error = xfs_alloc_vextent(&args);
+ if (error)
+ return error;
+
+ newlen = args.len << args.mp->m_sb.sb_inopblog;
+ allocmask = (1 << (newlen / XFS_INODES_PER_SPCHUNK)) - 1;
+ }
+
if (args.fsbno == NULLFSBLOCK) {
*alloc = 0;
return 0;
@@ -572,14 +600,34 @@ xfs_ialloc_ag_alloc(
/*
* Insert records describing the new inode chunk into the btrees.
*/
- error = xfs_inobt_insert(args.mp, tp, agbp, newino, newlen,
- XFS_INOBT_ALL_FREE, XFS_BTNUM_INO);
+ error = xfs_spchunk_has_record(args.mp, tp, agbp, newino, newlen,
+ XFS_BTNUM_INO, &rec);
+ if (error)
+ return error;
+ if (rec.ir_startino == NULLAGINO) {
+ /* no existing record, set all fields */
+ rec.ir_startino = newino;
+ rec.ir_holemask = ~allocmask;
+ rec.ir_freecount = newlen;
+ rec.ir_free = XFS_INOBT_ALL_FREE;
+ } else {
+ /* we already have a record, update it */
+ offset = newino - rec.ir_startino;
+ ASSERT(offset % XFS_INODES_PER_SPCHUNK == 0);
+
+ allocmask <<= offset / XFS_INODES_PER_SPCHUNK;
+
+ rec.ir_freecount += newlen;
+ rec.ir_holemask &= ~allocmask;
+ }
+
+ error = xfs_inobt_update_insert(args.mp, tp, agbp, &rec, XFS_BTNUM_INO);
if (error)
return error;

if (xfs_sb_version_hasfinobt(&args.mp->m_sb)) {
- error = xfs_inobt_insert(args.mp, tp, agbp, newino, newlen,
- XFS_INOBT_ALL_FREE, XFS_BTNUM_FINO);
+ error = xfs_inobt_update_insert(args.mp, tp, agbp, &rec,
+ XFS_BTNUM_FINO);
if (error)
return error;
}
@@ -1644,7 +1692,8 @@ xfs_difree_finobt(
*/
XFS_WANT_CORRUPTED_GOTO(ibtrec->ir_freecount == 1, error);

- error = xfs_inobt_insert_rec(cur, ibtrec->ir_freecount,
+ error = xfs_inobt_insert_rec(cur, ibtrec->ir_holemask,
+ ibtrec->ir_freecount,
ibtrec->ir_free, &i);
if (error)
goto error;

[Brian Foster]

v5 superblocks use an ordered log item for logging the initialization of
inode chunks. The icreate log item is currently hardcoded to an inode
count of 64 inodes.

The agbno and extent length are used to initialize the inode chunk from
log recovery. While an incorrect inode count does not lead to bad inode
chunk initialization, we should pass the correct inode count such that log
recovery has enough data to perform meaningful validity checks on the
chunk.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 7 ++++---
fs/xfs/libxfs/xfs_ialloc.h | 2 +-
fs/xfs/xfs_log_recover.c | 4 ++--
3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 8286fda..27d3437 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -223,6 +223,7 @@ xfs_ialloc_inode_init(
struct xfs_mount *mp,
struct xfs_trans *tp,
struct list_head *buffer_list,
+ int icount,
xfs_agnumber_t agno,
xfs_agblock_t agbno,
xfs_agblock_t length,
@@ -278,7 +279,7 @@ xfs_ialloc_inode_init(
* they track in the AIL as if they were physically logged.
*/
if (tp)
- xfs_icreate_log(tp, agno, agbno, mp->m_ialloc_inos,
+ xfs_icreate_log(tp, agno, agbno, icount,
mp->m_sb.sb_inodesize, length, gen);
} else
version = 2;
@@ -497,8 +498,8 @@ xfs_ialloc_ag_alloc(
* rather than a linear progression to prevent the next generation
* number from being easily guessable.
*/
- error = xfs_ialloc_inode_init(args.mp, tp, NULL, agno, args.agbno,
- args.len, prandom_u32());
+ error = xfs_ialloc_inode_init(args.mp, tp, NULL, newlen, agno,
+ args.agbno, args.len, prandom_u32());

if (error)
return error;
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 95ad1c0..62c1381 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -156,7 +156,7 @@ int xfs_inobt_get_rec(struct xfs_btree_cur *cur,
* Inode chunk initialisation routine
*/
int xfs_ialloc_inode_init(struct xfs_mount *mp, struct xfs_trans *tp,
- struct list_head *buffer_list,
+ struct list_head *buffer_list, int icount,
xfs_agnumber_t agno, xfs_agblock_t agbno,
xfs_agblock_t length, unsigned int gen);

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index fbc2362..cbe782a 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3236,8 +3236,8 @@ xlog_recover_do_icreate_pass2(
XFS_AGB_TO_DADDR(mp, agno, agbno), length, 0))
return 0;

- xfs_ialloc_inode_init(mp, NULL, buffer_list, agno, agbno, length,
- be32_to_cpu(icl->icl_gen));
+ xfs_ialloc_inode_init(mp, NULL, buffer_list, count, agno, agbno, length,
+ be32_to_cpu(icl->icl_gen));
return 0;
}

[Brian Foster]

Enable the use and processing of sparse inode chunks. Fix the
xfs_sb_version_hassparseinodes() helper to allow the allocation of
sparse chunks on v5 format superblocks. Add the incompat. feature bit to
the *_ALL mask such that fs' with sparse chunks can be mounted.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_sb.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
index 6f48de9..f2299b7 100644
--- a/fs/xfs/libxfs/xfs_sb.h
+++ b/fs/xfs/libxfs/xfs_sb.h
@@ -511,7 +511,8 @@ xfs_sb_has_ro_compat_feature(
#define XFS_SB_FEAT_INCOMPAT_FTYPE (1 << 0) /* filetype in dirent */
#define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */
#define XFS_SB_FEAT_INCOMPAT_ALL \
- (XFS_SB_FEAT_INCOMPAT_FTYPE)
+ (XFS_SB_FEAT_INCOMPAT_FTYPE| \
+ XFS_SB_FEAT_INCOMPAT_SPINODES)

#define XFS_SB_FEAT_INCOMPAT_UNKNOWN ~XFS_SB_FEAT_INCOMPAT_ALL
static inline bool
@@ -561,7 +562,7 @@ static inline int xfs_sb_version_hasfinobt(xfs_sb_t *sbp)

static inline int xfs_sb_version_hassparseinodes(struct xfs_sb *sbp)
{
- return 0; /* not yet enabled */
+ return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
}

/*

[Dave Chinner]

Post by Brian Foster
Enable the use and processing of sparse inode chunks. Fix the
xfs_sb_version_hassparseinodes() helper to allow the allocation of
sparse chunks on v5 format superblocks. Add the incompat. feature bit to
the *_ALL mask such that fs' with sparse chunks can be mounted.

Interesting way of allowing XFS_SB_FEAT_INCOMPAT_SPINODES to be
optional. :)

I didn't expect it to be used this way, hence my earlier comments
about chicken and egg feature bits. I still think I'd like this to
be explicit rather than just added when the format first changes...

Cheers,

Dave.

[Brian Foster]

The inode btrees track 64 inodes per record, regardless of inode size.
Thus, inode chunks on disk vary in size depending on the size of the
inodes. This creates a contiguous allocation requirement for new inode
chunks that can be difficult to satisfy on an aged and fragmented (free
space) filesystem.

The inode record freecount currently uses 4 bytes on disk to track the
free inode count. With a maximum freecount value of 64, only one byte is
required. Convert the freecount field to a single byte and reserve two
of the remaining 3 higher order bytes left to the hole mask field.

The hole mask field tracks potential holes in the chunks of physical
space that the inode record refers to. This facilitates the sparse
allocation of inode chunks when contiguous chunks are not available and
allows the inode btrees to identify what portions of the chunk contain
valid inodes.

Tracking holes means the field is initialized to zero and thus maintains
backwards compatibility with existing filesystems. E.g., the higher
order bytes of a counter with a max value of 64 are already initialized
to 0. Update the inode record management functions to handle the new
field and initialize it to zero for now.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_format.h | 7 +++++--
fs/xfs/libxfs/xfs_ialloc.c | 9 +++++++--
fs/xfs/libxfs/xfs_ialloc_btree.c | 4 +++-
3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 34d85ac..39022d9 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -221,13 +221,16 @@ static inline xfs_inofree_t xfs_inobt_maskn(int i, int n)
*/
typedef struct xfs_inobt_rec {
__be32 ir_startino; /* starting inode number */
- __be32 ir_freecount; /* count of free inodes (set bits) */
+ __be16 ir_holemask; /* hole mask for sparse chunks */
+ __u8 ir_pad;
+ __u8 ir_freecount; /* count of free inodes (set bits) */
__be64 ir_free; /* free inode mask */
} xfs_inobt_rec_t;

typedef struct xfs_inobt_rec_incore {
xfs_agino_t ir_startino; /* starting inode number */
- __int32_t ir_freecount; /* count of free inodes (set bits) */
+ __uint16_t ir_holemask; /* hole mask for sparse chunks */
+ __uint8_t ir_freecount; /* count of free inodes (set bits) */
xfs_inofree_t ir_free; /* free inode mask */
} xfs_inobt_rec_incore_t;

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 5448a74..8286fda 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -68,6 +68,7 @@ xfs_inobt_lookup(
int *stat) /* success/failure */
{
cur->bc_rec.i.ir_startino = ino;
+ cur->bc_rec.i.ir_holemask = 0;
cur->bc_rec.i.ir_freecount = 0;
cur->bc_rec.i.ir_free = 0;
return xfs_btree_lookup(cur, dir, stat);
@@ -85,7 +86,9 @@ xfs_inobt_update(
union xfs_btree_rec rec;

rec.inobt.ir_startino = cpu_to_be32(irec->ir_startino);
- rec.inobt.ir_freecount = cpu_to_be32(irec->ir_freecount);
+ rec.inobt.ir_holemask = cpu_to_be16(irec->ir_holemask);
+ rec.inobt.ir_pad = 0;
+ rec.inobt.ir_freecount = irec->ir_freecount;
rec.inobt.ir_free = cpu_to_be64(irec->ir_free);
return xfs_btree_update(cur, &rec);
}
@@ -105,7 +108,8 @@ xfs_inobt_get_rec(
error = xfs_btree_get_rec(cur, &rec, stat);
if (!error && *stat == 1) {
irec->ir_startino = be32_to_cpu(rec->inobt.ir_startino);
- irec->ir_freecount = be32_to_cpu(rec->inobt.ir_freecount);
+ irec->ir_holemask = be16_to_cpu(rec->inobt.ir_holemask);
+ irec->ir_freecount = rec->inobt.ir_freecount;
irec->ir_free = be64_to_cpu(rec->inobt.ir_free);
}
return error;
@@ -121,6 +125,7 @@ xfs_inobt_insert_rec(
xfs_inofree_t free,
int *stat)
{
+ cur->bc_rec.i.ir_holemask = 0;
cur->bc_rec.i.ir_freecount = freecount;
cur->bc_rec.i.ir_free = free;
return xfs_btree_insert(cur, stat);
diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index c9b06f3..0c94879 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -169,7 +169,9 @@ xfs_inobt_init_rec_from_cur(
union xfs_btree_rec *rec)
{
rec->inobt.ir_startino = cpu_to_be32(cur->bc_rec.i.ir_startino);
- rec->inobt.ir_freecount = cpu_to_be32(cur->bc_rec.i.ir_freecount);
+ rec->inobt.ir_holemask = cpu_to_be16(cur->bc_rec.i.ir_holemask);
+ rec->inobt.ir_pad = 0;
+ rec->inobt.ir_freecount = cur->bc_rec.i.ir_freecount;
rec->inobt.ir_free = cpu_to_be64(cur->bc_rec.i.ir_free);
}

[Dave Chinner]

Post by Brian Foster
The inode btrees track 64 inodes per record, regardless of inode size.
Thus, inode chunks on disk vary in size depending on the size of the
inodes. This creates a contiguous allocation requirement for new inode
chunks that can be difficult to satisfy on an aged and fragmented (free
space) filesystem.
The inode record freecount currently uses 4 bytes on disk to track the
free inode count. With a maximum freecount value of 64, only one byte is
required. Convert the freecount field to a single byte and reserve two
of the remaining 3 higher order bytes left to the hole mask field.
The hole mask field tracks potential holes in the chunks of physical
space that the inode record refers to. This facilitates the sparse
allocation of inode chunks when contiguous chunks are not available and
allows the inode btrees to identify what portions of the chunk contain
valid inodes.
Tracking holes means the field is initialized to zero and thus maintains
backwards compatibility with existing filesystems. E.g., the higher
order bytes of a counter with a max value of 64 are already initialized
to 0. Update the inode record management functions to handle the new
field and initialize it to zero for now.
---
fs/xfs/libxfs/xfs_format.h | 7 +++++--
fs/xfs/libxfs/xfs_ialloc.c | 9 +++++++--
fs/xfs/libxfs/xfs_ialloc_btree.c | 4 +++-
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 34d85ac..39022d9 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -221,13 +221,16 @@ static inline xfs_inofree_t xfs_inobt_maskn(int i, int n)
*/
typedef struct xfs_inobt_rec {
__be32 ir_startino; /* starting inode number */
- __be32 ir_freecount; /* count of free inodes (set bits) */
+ __be16 ir_holemask; /* hole mask for sparse chunks */
+ __u8 ir_pad;
+ __u8 ir_freecount; /* count of free inodes (set bits) */
__be64 ir_free; /* free inode mask */
} xfs_inobt_rec_t;

might we want the number of inodes allocated in the chunk there as
well (i.e. in the pad field) so we can validate the holemask against
the number of inodes allocated in the chunk?

-Dave.

[Brian Foster]

Post by Dave Chinner

Post by Brian Foster
The inode btrees track 64 inodes per record, regardless of inode size.
Thus, inode chunks on disk vary in size depending on the size of the
inodes. This creates a contiguous allocation requirement for new inode
chunks that can be difficult to satisfy on an aged and fragmented (free
space) filesystem.
The inode record freecount currently uses 4 bytes on disk to track the
free inode count. With a maximum freecount value of 64, only one byte is
required. Convert the freecount field to a single byte and reserve two
of the remaining 3 higher order bytes left to the hole mask field.
The hole mask field tracks potential holes in the chunks of physical
space that the inode record refers to. This facilitates the sparse
allocation of inode chunks when contiguous chunks are not available and
allows the inode btrees to identify what portions of the chunk contain
valid inodes.
Tracking holes means the field is initialized to zero and thus maintains
backwards compatibility with existing filesystems. E.g., the higher
order bytes of a counter with a max value of 64 are already initialized
to 0. Update the inode record management functions to handle the new
field and initialize it to zero for now.
---
fs/xfs/libxfs/xfs_format.h | 7 +++++--
fs/xfs/libxfs/xfs_ialloc.c | 9 +++++++--
fs/xfs/libxfs/xfs_ialloc_btree.c | 4 +++-
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 34d85ac..39022d9 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -221,13 +221,16 @@ static inline xfs_inofree_t xfs_inobt_maskn(int i, int n)
*/
typedef struct xfs_inobt_rec {
__be32 ir_startino; /* starting inode number */
- __be32 ir_freecount; /* count of free inodes (set bits) */
+ __be16 ir_holemask; /* hole mask for sparse chunks */
+ __u8 ir_pad;
+ __u8 ir_freecount; /* count of free inodes (set bits) */
__be64 ir_free; /* free inode mask */
} xfs_inobt_rec_t;

might we want the number of inodes allocated in the chunk there as
well (i.e. in the pad field) so we can validate the holemask against
the number of inodes allocated in the chunk?

So you're suggesting something like this?

- __be32 ir_freecount; /* count of free inodes (set bits) */
+ __be16 ir_holemask; /* hole mask for sparse chunks */
+ __u8 ir_count; /* total inode count */
+ __u8 ir_freecount; /* count of free inodes (set bits) */

That's an interesting thought. It might make some of the code more clear
and eliminate the need for the derivation of that value from the
holemask (beyond for validation purposes). I do like the extra
validation and potential debug use given the holemask is not quite as
human friendly as the free mask in terms of having a bit per inode.

As long as there isn't any concern over reserving this space for
something else down the road (I suspect not, since the pad is introduced
by this feature), I'll look to use it as an inode count.

Brian

Post by Dave Chinner
-Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Brian Foster]

Post by Brian Foster

Post by Dave Chinner

Post by Brian Foster
The inode btrees track 64 inodes per record, regardless of inode size.
Thus, inode chunks on disk vary in size depending on the size of the
inodes. This creates a contiguous allocation requirement for new inode
chunks that can be difficult to satisfy on an aged and fragmented (free
space) filesystem.
The inode record freecount currently uses 4 bytes on disk to track the
free inode count. With a maximum freecount value of 64, only one byte is
required. Convert the freecount field to a single byte and reserve two
of the remaining 3 higher order bytes left to the hole mask field.
The hole mask field tracks potential holes in the chunks of physical
space that the inode record refers to. This facilitates the sparse
allocation of inode chunks when contiguous chunks are not available and
allows the inode btrees to identify what portions of the chunk contain
valid inodes.
Tracking holes means the field is initialized to zero and thus maintains
backwards compatibility with existing filesystems. E.g., the higher
order bytes of a counter with a max value of 64 are already initialized
to 0. Update the inode record management functions to handle the new
field and initialize it to zero for now.
---
fs/xfs/libxfs/xfs_format.h | 7 +++++--
fs/xfs/libxfs/xfs_ialloc.c | 9 +++++++--
fs/xfs/libxfs/xfs_ialloc_btree.c | 4 +++-
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 34d85ac..39022d9 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -221,13 +221,16 @@ static inline xfs_inofree_t xfs_inobt_maskn(int i, int n)
*/
typedef struct xfs_inobt_rec {
__be32 ir_startino; /* starting inode number */
- __be32 ir_freecount; /* count of free inodes (set bits) */
+ __be16 ir_holemask; /* hole mask for sparse chunks */
+ __u8 ir_pad;
+ __u8 ir_freecount; /* count of free inodes (set bits) */
__be64 ir_free; /* free inode mask */
} xfs_inobt_rec_t;

might we want the number of inodes allocated in the chunk there as
well (i.e. in the pad field) so we can validate the holemask against
the number of inodes allocated in the chunk?

So you're suggesting something like this?
- __be32 ir_freecount; /* count of free inodes (set bits) */
+ __be16 ir_holemask; /* hole mask for sparse chunks */
+ __u8 ir_count; /* total inode count */
+ __u8 ir_freecount; /* count of free inodes (set bits) */
That's an interesting thought. It might make some of the code more clear
and eliminate the need for the derivation of that value from the
holemask (beyond for validation purposes). I do like the extra
validation and potential debug use given the holemask is not quite as
human friendly as the free mask in terms of having a bit per inode.
As long as there isn't any concern over reserving this space for
something else down the road (I suspect not, since the pad is introduced
by this feature), I'll look to use it as an inode count.

The ir_count field along with changing the way we handle the feature bit
to be fixed rather than dynamic introduces an interesting design point
with regard to backwards compatibility.

The feature bit is now fixed at mkfs time and we have an assumption that
it could be removed via some userspace mechanism (e.g., repair,
xfs_admin, etc.). To do that safely, the mechanism requires some kind of
verification that sparse inode chunks do not exist, either as part of
the repair tracking or some kind of new counter somewhere that xfs_admin
could refer to.

The ir_count field helps with verification of the holemask and
simplifies calculation of a "real" inode count for such records (e.g.,
for bulkstat). We could set ir_count only for inode records that are
sparse, but that seems slightly unfortunate if we expect this record
format to eventually be default. Alternatively (and what I have in my
tree at the moment), we can use ir_count unconditionally so long as the
feature bit is set. That obviously breaks the backwards compatibility of
the record format since this is a higher order byte of ir_freecount on
older kernels.

So if the feature bit is mkfs time and there is no intent to enable it
thereafter, it seems there isn't much benefit to record backwards
compatibility. If we go that route, perhaps it's better to have an
ir_alloc field or some such instead of ir_holemask, invert the bits and
eliminate that bit of complexity. On the flipside, limiting usage of
ir_count and retaining ir_holemask in its current form leaves open the
possibility for enabling sparse inode chunks without a reformat (given
certain requirements, e.g., a v5 superblock). Thoughts?

Brian

Post by Brian Foster
Brian

Post by Dave Chinner
-Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Brian Foster]

Sparse inode chunks allow the traditional inode btree record format to
describe an inode chunk that is not fully allocated and/or contiguous.
Define a couple constants that set requirements for allocation and
management of such chunks. Also define a helper to easily detect sparse
inode chunks.

The granularity of a sparse chunk is defined by the the 16-bit holemask
field in the inode record. Assuming 64 inodes per full chunk, a single
holemask bit accounts for 4 inodes. The minimum allocation requirement
for a sparse inode chunk is defined as the minimum number of blocks
required to meet the 4 inode granularity.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_format.h | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 39022d9..0baad50 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -211,6 +211,11 @@ typedef __uint64_t xfs_inofree_t;
#define XFS_INOBT_ALL_FREE ((xfs_inofree_t)-1)
#define XFS_INOBT_MASK(i) ((xfs_inofree_t)1 << (i))

+#define XFS_INODES_PER_SPCHUNK \
+ (XFS_INODES_PER_CHUNK / (NBBY * sizeof(__uint16_t)))
+#define XFS_INOBT_MIN_SPCHUNKLEN(sb) \
+ (roundup(XFS_INODES_PER_SPCHUNK, sb.sb_inopblock) / sb.sb_inopblock)
+
static inline xfs_inofree_t xfs_inobt_maskn(int i, int n)
{
return ((n >= XFS_INODES_PER_CHUNK ? 0 : XFS_INOBT_MASK(n)) - 1) << i;
@@ -234,6 +239,10 @@ typedef struct xfs_inobt_rec_incore {
xfs_inofree_t ir_free; /* free inode mask */
} xfs_inobt_rec_incore_t;

+static inline bool xfs_inobt_issparse(struct xfs_inobt_rec_incore *rec)
+{
+ return rec->ir_holemask == 0 ? false : true;
+}

/*
* Key structure

[Dave Chinner]

Post by Brian Foster
Sparse inode chunks allow the traditional inode btree record format to
describe an inode chunk that is not fully allocated and/or contiguous.
Define a couple constants that set requirements for allocation and
management of such chunks. Also define a helper to easily detect sparse
inode chunks.
The granularity of a sparse chunk is defined by the the 16-bit holemask
field in the inode record. Assuming 64 inodes per full chunk, a single
holemask bit accounts for 4 inodes. The minimum allocation requirement
for a sparse inode chunk is defined as the minimum number of blocks
required to meet the 4 inode granularity.
---
fs/xfs/libxfs/xfs_format.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 39022d9..0baad50 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -211,6 +211,11 @@ typedef __uint64_t xfs_inofree_t;
#define XFS_INOBT_ALL_FREE ((xfs_inofree_t)-1)
#define XFS_INOBT_MASK(i) ((xfs_inofree_t)1 << (i))
+#define XFS_INODES_PER_SPCHUNK \
+ (XFS_INODES_PER_CHUNK / (NBBY * sizeof(__uint16_t)))
+#define XFS_INOBT_MIN_SPCHUNKLEN(sb) \
+ (roundup(XFS_INODES_PER_SPCHUNK, sb.sb_inopblock) / sb.sb_inopblock)

static inline.

-Dave.

[Brian Foster]

The bulkstat and inumbers mechanisms make the assumption that inode
records consist of a full 64 inode chunk in several places. E.g., this
is used to track how many inodes have been processed overall as well as
to determine when all in-use inodes of a record have been processed. The
record processing, in particular, increments the record freecount for
each in-use inode until it hits the expected max of 64.

This is invalid for sparse inode records. While all inodes might be
marked free in the free mask regardless of whether they are allocated on
disk, ir_freecount is based on the total number of physically allocated
inodes and thus may be less than 64 inodes on a completely free inode
chunk.

Create the xfs_inobt_count() helper to calculate the total number of
physically allocated inodes based on the holemask. Use the helper in
xfs_bulkstat() and xfs_inumbers() instead of the fixed
XFS_INODE_PER_CHUNK value to ensure correct accounting in the event of
sparse inode records.

Signed-off-by: Brian Foster <***@redhat.com>
---
fs/xfs/libxfs/xfs_ialloc.c | 27 +++++++++++++++++++++++++++
fs/xfs/libxfs/xfs_ialloc.h | 5 +++++
fs/xfs/xfs_itable.c | 12 +++++++-----
3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 86c6ccd..daf317f 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -951,6 +951,33 @@ xfs_inobt_first_free_inode(
}

/*
+ * Calculate the real count of inodes in a chunk.
+ */
+int
+xfs_inobt_count(
+ struct xfs_inobt_rec_incore *rec)
+{
+ __uint16_t allocmask;
+ uint allocbitmap;
+ int nextbit;
+ int count = 0;
+
+ if (!xfs_inobt_issparse(rec))
+ return XFS_INODES_PER_CHUNK;
+
+ allocmask = ~rec->ir_holemask;
+ allocbitmap = allocmask;
+
+ nextbit = xfs_next_bit(&allocbitmap, 1, 0);
+ while (nextbit != -1) {
+ count += XFS_INODES_PER_SPCHUNK;
+ nextbit = xfs_next_bit(&allocbitmap, 1, nextbit + 1);
+ }
+
+ return count;
+}
+
+/*
* Allocate an inode using the inobt-only algorithm.
*/
STATIC int
diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h
index 5aa8d6f..4230b22 100644
--- a/fs/xfs/libxfs/xfs_ialloc.h
+++ b/fs/xfs/libxfs/xfs_ialloc.h
@@ -166,4 +166,9 @@ int xfs_ialloc_inode_init(struct xfs_mount *mp, struct xfs_trans *tp,
xfs_agnumber_t agno, xfs_agblock_t agbno,
xfs_agblock_t length, unsigned int gen);

+/*
+ * Calculate the real count of inodes in a chunk.
+ */
+int xfs_inobt_count(struct xfs_inobt_rec_incore *rec);
+
#endif /* __XFS_IALLOC_H__ */
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 7e54992..8ba967c 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -312,11 +312,12 @@ xfs_bulkstat(
}
r.ir_free |= xfs_inobt_maskn(0, chunkidx);
irbp->ir_startino = r.ir_startino;
+ irbp->ir_holemask = r.ir_holemask;
irbp->ir_freecount = r.ir_freecount;
irbp->ir_free = r.ir_free;
irbp++;
agino = r.ir_startino + XFS_INODES_PER_CHUNK;
- icount = XFS_INODES_PER_CHUNK - r.ir_freecount;
+ icount = xfs_inobt_count(&r) - r.ir_freecount;
} else {
/*
* If any of those tests failed, bump the
@@ -376,7 +377,7 @@ xfs_bulkstat(
* If this chunk has any allocated inodes, save it.
* Also start read-ahead now for this chunk.
*/
- if (r.ir_freecount < XFS_INODES_PER_CHUNK) {
+ if (r.ir_freecount < xfs_inobt_count(&r)) {
struct blk_plug plug;
/*
* Loop over all clusters in the next chunk.
@@ -397,10 +398,11 @@ xfs_bulkstat(
}
blk_finish_plug(&plug);
irbp->ir_startino = r.ir_startino;
+ irbp->ir_holemask = r.ir_holemask;
irbp->ir_freecount = r.ir_freecount;
irbp->ir_free = r.ir_free;
irbp++;
- icount += XFS_INODES_PER_CHUNK - r.ir_freecount;
+ icount += xfs_inobt_count(&r) - r.ir_freecount;
}
/*
* Set agino to after this chunk and bump the cursor.
@@ -427,7 +429,7 @@ xfs_bulkstat(
*/
for (agino = irbp->ir_startino, chunkidx = clustidx = 0;
XFS_BULKSTAT_UBLEFT(ubleft) &&
- irbp->ir_freecount < XFS_INODES_PER_CHUNK;
+ irbp->ir_freecount < xfs_inobt_count(irbp);
chunkidx++, clustidx++, agino++) {
ASSERT(chunkidx < XFS_INODES_PER_CHUNK);

@@ -654,7 +656,7 @@ xfs_inumbers(
buffer[bufidx].xi_startino =
XFS_AGINO_TO_INO(mp, agno, r.ir_startino);
buffer[bufidx].xi_alloccount =
- XFS_INODES_PER_CHUNK - r.ir_freecount;
+ xfs_inobt_count(&r) - r.ir_freecount;
buffer[bufidx].xi_allocmask = ~r.ir_free;
bufidx++;
left--;

[Dave Chinner]

Post by Brian Foster
The bulkstat and inumbers mechanisms make the assumption that inode
records consist of a full 64 inode chunk in several places. E.g., this
is used to track how many inodes have been processed overall as well as
to determine when all in-use inodes of a record have been processed. The
record processing, in particular, increments the record freecount for
each in-use inode until it hits the expected max of 64.
This is invalid for sparse inode records. While all inodes might be
marked free in the free mask regardless of whether they are allocated on
disk, ir_freecount is based on the total number of physically allocated
inodes and thus may be less than 64 inodes on a completely free inode
chunk.
Create the xfs_inobt_count() helper to calculate the total number of
physically allocated inodes based on the holemask. Use the helper in
xfs_bulkstat() and xfs_inumbers() instead of the fixed
XFS_INODE_PER_CHUNK value to ensure correct accounting in the event of
sparse inode records.
---
fs/xfs/libxfs/xfs_ialloc.c | 27 +++++++++++++++++++++++++++
fs/xfs/libxfs/xfs_ialloc.h | 5 +++++
fs/xfs/xfs_itable.c | 12 +++++++-----
3 files changed, 39 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 86c6ccd..daf317f 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -951,6 +951,33 @@ xfs_inobt_first_free_inode(
}
/*
+ * Calculate the real count of inodes in a chunk.
+ */
+int
+xfs_inobt_count(
+ struct xfs_inobt_rec_incore *rec)
+{
+ __uint16_t allocmask;
+ uint allocbitmap;
+ int nextbit;
+ int count = 0;
+
+ if (!xfs_inobt_issparse(rec))
+ return XFS_INODES_PER_CHUNK;
+
+ allocmask = ~rec->ir_holemask;
+ allocbitmap = allocmask;
+
+ nextbit = xfs_next_bit(&allocbitmap, 1, 0);
+ while (nextbit != -1) {
+ count += XFS_INODES_PER_SPCHUNK;
+ nextbit = xfs_next_bit(&allocbitmap, 1, nextbit + 1);
+ }

bitmap_weight() * XFS_INODES_PER_SPCHUNK?

-Dave

[Brian Foster]

Hi all,
This is a first pass at sparse inode chunk support for XFS. Some
http://oss.sgi.com/archives/xfs/2013-08/msg00346.html
The basic idea is to allow the partial allocation of inode chunks into
fragmented regions of free space. This is accomplished through addition
of a holemask field into the inobt record that defines what portion(s)
of an inode chunk are invalid (i.e., holes in the chunk). This work is
not quite complete, but is at a point where I'd like to start getting
feedback on the design and what direction to take for some of the known
gaps.

I've attached a tarball to this message with a couple userspace patches
and an xfstests patch to facilitate experimentation. The userspace
patches update the inobt record data structure and add the holemask
field to xfs_db to facilitate poking around. Note that the rest of
userspace is untouched at this point, including repair being broken,
etc., so I don't recommend use beyond xfs_db.

The xfstests test case fragments free space, allocates inodes until
ENOSPC and expects to consume most of the free space available in the
fs. The "fragmentation factor" is currently dynamic and based on the
cluster size due to the cluster size scaling behavior documented below.

Finally, sparse inode chunks are only enabled for v5 superblocks, so a
crc enabled fs is required to test.

Brian

- Patches 1-2 - A couple generic cleanups that are dependencies for later
patches in the series.
- Patches 3-5 - Basic data structure update, feature bit and minor
helper introduction.
- Patches 6-7 - Update v5 icreate logging and recovery to handle sparse
inode records.
- Patches 8-13 - Allocation support for sparse inode records. Physical
chunk allocation and individual inode allocation.
- Patches 14-16 - Deallocation support for sparse inode chunks. Physical
chunk deallocation, individual inode free and cluster free.
- Patch 17 - Fixes for bulkstat/inumbers.
- Patch 18 - Activate support for sparse chunk allocation and
processing.
This work is lightly tested for regression (some xfstests failures due
to repair) and basic functionality. I have a new xfstests test I'll
forward along for demonstration purposes.
The current minimum sparse chunk allocation granularity is the cluster
size. My initial attempts at this work tried to redefine to the minimum
chunk length based on the holemask granularity (a la the stale macro I
seemingly left in this series ;), but this involves tweaking the
codepaths that use the cluster size (i.e., imap) which proved rather
hairy. This also means we need a solution where an imap can change if an
inode was initially mapped as a sparse chunk and said chunk is
subsequently made full. E.g., we'd perhaps need to invalidate the inode
buffers for sparse chunks at the time where they are made full. Given
that, I landed on using the cluster size and leaving those codepaths as
is for the time being.
There is a tradeoff here for v5 superblocks because we've recently made
a change to scale the cluster size based on the factor increase in the
inode size from the default (see xfsprogs commit 7b5f9801). This means
that effectiveness of sparse chunks is tied to whether the level of free
space fragmentation matches the cluster size. By that I mean effectivess
is good (near 100% utilization possible) if free space fragmentation
leaves free extents around that at least match the cluster size. If
fragmentation is worse than the cluster size, effectiveness is reduced.
This can also be demonstrated with the forthcoming xfstests test.
We set an incompatible feature bit once a sparse inode chunk is
allocated because older revisions of code will interpret the non-zero
holemask bits in the higher order bytes of the record freecount. The
feature bit must be removed once all sparse inode chunks are eliminated
one way or another. This series does not currently remove the feature
bit once set simply because I hadn't thought through the mechanism quite
yet. For the next version, I'm thinking about adding an inobt walk
mechanism that can be conditionally invoked (i.e., feature bit is
currently set and a sparse inode chunk has been eliminated) either via
workqueue on an interval or during unmount if necessary. Thoughts or
alternative suggestions on that appreciated.
That's about it for now. Thoughts, reviews, flames appreciated. Thanks.
Brian
xfs: refactor xfs_inobt_insert() to eliminate loop and support
variable count
xfs: pass xfs_mount directly to xfs_ialloc_cluster_alignment()
xfs: define sparse inode chunks v5 sb feature bit and helper function
xfs: introduce inode record hole mask for sparse inode chunks
xfs: create macros/helpers for dealing with sparse inode chunks
xfs: pass inode count through ordered icreate log item
xfs: handle sparse inode chunks in icreate log recovery
xfs: create helper to manage record overlap for sparse inode chunks
xfs: allocate sparse inode chunks on full chunk allocation failure
xfs: set sparse inodes feature bit when a sparse chunk is allocated
xfs: reduce min. inode allocation space requirement for sparse inode
chunks
xfs: helper to convert inobt record holemask to inode alloc. bitmap
xfs: filter out sparse regions from individual inode allocation
xfs: update free inode record logic to support sparse inode records
xfs: only free allocated regions of inode chunks
xfs: skip unallocated regions of inode chunks in xfs_ifree_cluster()
xfs: use actual inode count for sparse records in bulkstat/inumbers
xfs: enable sparse inode chunks for v5 superblocks
fs/xfs/libxfs/xfs_format.h | 17 +-
fs/xfs/libxfs/xfs_ialloc.c | 441 +++++++++++++++++++++++++++++++++------
fs/xfs/libxfs/xfs_ialloc.h | 17 +-
fs/xfs/libxfs/xfs_ialloc_btree.c | 4 +-
fs/xfs/libxfs/xfs_sb.h | 9 +-
fs/xfs/xfs_inode.c | 28 ++-
fs/xfs/xfs_itable.c | 12 +-
fs/xfs/xfs_log_recover.c | 23 +-
8 files changed, 460 insertions(+), 91 deletions(-)
--
1.8.3.1
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Dave Chinner]

Post by Brian Foster
Inodes are always allocated in chunks of 64 and thus the loop in
xfs_inobt_insert() is unnecessary.

I don't believe this is true. The number of inodes allocated at once
is:

mp->m_ialloc_inos = (int)MAX((__uint16_t)XFS_INODES_PER_CHUNK,
sbp->sb_inopblock);

So when the block size is, say, 64k, the number of 512 byte inodes
allocated at once is 128. i.e. 2 chunks. Hence xfs_inobt_insert()
can be called with a inode could of > 64 and therefore the loop is
still necessary...

And, indeed, we might want to increase the allocation size in future
to do entire stripe units or stripe widths of inodes at once:

http://xfs.org/index.php/Improving_inode_Caching#Contiguous_Inode_Allocation

This also means a loop would be required -somewhere-...

Cheers,

Dave.

[Brian Foster]

Post by Dave Chinner

Post by Brian Foster
Inodes are always allocated in chunks of 64 and thus the loop in
xfs_inobt_insert() is unnecessary.

I don't believe this is true. The number of inodes allocated at once
mp->m_ialloc_inos = (int)MAX((__uint16_t)XFS_INODES_PER_CHUNK,
sbp->sb_inopblock);

So I'm going on that effectively that the number of inodes per block
will never be larger than 8 (v5) due to a max block size of 4k.

Post by Dave Chinner
So when the block size is, say, 64k, the number of 512 byte inodes
allocated at once is 128. i.e. 2 chunks. Hence xfs_inobt_insert()
can be called with a inode could of > 64 and therefore the loop is
still necessary...

Playing with mkfs I see that we actually can format >4k bsize
filesystems and the min and max are set at 512b and 64k. I can't
actually mount such filesystems due to the page size limitation. FWIW,
the default log size params appear to be broken for bsize >= 32k as
well, so I wonder if/how often that format tends to occur.

What's the situation with regard to >PAGE_SIZE block size support? Is
this something we actually could support today? Do we know about any
large page sized arches that could push us into this territory with the
actual page size limitation?

Post by Dave Chinner
And, indeed, we might want to increase the allocation size in future
http://xfs.org/index.php/Improving_inode_Caching#Contiguous_Inode_Allocation
This also means a loop would be required -somewhere-...

Indeed, though I'm less inclined to keep this around for the purposes of
this unimplemented feature. It should be easy enough to add the loop in
the appropriate place according to the code at the time this is
implemented.

I suppose if we have >4k page sized arches that utilize block sizes
outside of the 256b-4k range, that's enough to justify the existence of
the range in the general sense. I just might have to factor this area of
code a bit differently. It would also be nice if there was a means to
test.

Brian

Post by Dave Chinner
Cheers,
Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Dave Chinner]

Post by Brian Foster

Post by Dave Chinner

Post by Brian Foster
Inodes are always allocated in chunks of 64 and thus the loop in
xfs_inobt_insert() is unnecessary.

I don't believe this is true. The number of inodes allocated at once
mp->m_ialloc_inos = (int)MAX((__uint16_t)XFS_INODES_PER_CHUNK,
sbp->sb_inopblock);

So I'm going on that effectively that the number of inodes per block
will never be larger than 8 (v5) due to a max block size of 4k.

The whole world is not x86... ;)

Post by Brian Foster

Post by Dave Chinner
So when the block size is, say, 64k, the number of 512 byte inodes
allocated at once is 128. i.e. 2 chunks. Hence xfs_inobt_insert()
can be called with a inode could of > 64 and therefore the loop is
still necessary...

Playing with mkfs I see that we actually can format >4k bsize
filesystems and the min and max are set at 512b and 64k. I can't
actually mount such filesystems due to the page size limitation.

The whole world is not x86.... ;)

ia64 and power default to 64k page size, so we have to code
everything to work with 64k block sizes.

Post by Brian Foster
FWIW,
the default log size params appear to be broken for bsize >= 32k as

In what way?

Post by Brian Foster
well, so I wonder if/how often that format tends to occur.

More often than you think.

Post by Brian Foster
What's the situation with regard to >PAGE_SIZE block size support? Is
this something we actually could support today?

Well, the problem is bufferheads and page cache don't support blocks
large than page size. The metadata side of XFS supports it just fine
through the xfs_buf structures, but the file data side doesn't.
That's one of the things I'm slowly trying to find time to fix (i.e.
kill bufferheads).

Post by Brian Foster
Do we know about any
large page sized arches that could push us into this territory with the
actual page size limitation?

Yes, see above. We have always supported 64k block sizes on Linux
ever since ia64 supported 64k page sizes (i.e. for at least 10
years), so we can't now say "we only support 4k block sizes"....

Post by Brian Foster
I suppose if we have >4k page sized arches that utilize block sizes
outside of the 256b-4k range, that's enough to justify the existence of
the range in the general sense. I just might have to factor this area of
code a bit differently. It would also be nice if there was a means to
test.

Grab a ppc64 box from the RH QE guys or ask them to test it....

Cheers,

Dave.

[Brian Foster]

Post by Dave Chinner

Post by Brian Foster

Post by Dave Chinner

Post by Brian Foster
Inodes are always allocated in chunks of 64 and thus the loop in
xfs_inobt_insert() is unnecessary.

I don't believe this is true. The number of inodes allocated at once
mp->m_ialloc_inos = (int)MAX((__uint16_t)XFS_INODES_PER_CHUNK,
sbp->sb_inopblock);

So I'm going on that effectively that the number of inodes per block
will never be larger than 8 (v5) due to a max block size of 4k.

The whole world is not x86... ;)

Post by Brian Foster

Post by Dave Chinner
So when the block size is, say, 64k, the number of 512 byte inodes
allocated at once is 128. i.e. 2 chunks. Hence xfs_inobt_insert()
can be called with a inode could of > 64 and therefore the loop is
still necessary...

Playing with mkfs I see that we actually can format >4k bsize
filesystems and the min and max are set at 512b and 64k. I can't
actually mount such filesystems due to the page size limitation.

The whole world is not x86.... ;)
ia64 and power default to 64k page size, so we have to code
everything to work with 64k block sizes.

Yeah, I was aware there are >4k paged arches. I just wasn't sure which
and how they're used. I'll look into these.

Post by Dave Chinner

Post by Brian Foster
FWIW,
the default log size params appear to be broken for bsize >= 32k as

In what way?

# mkfs.xfs -f /dev/test/scratch -bsize=32k
log size 320 blocks too small, minimum size is 512 blocks
Usage: mkfs.xfs
...

This is a 10G lv so I suspect the following code is related:

...
} else if (dblocks < GIGABYTES(16, blocklog)) {
...
logblocks = MIN(XFS_MIN_LOG_BYTES >> blocklog,
min_logblocks * XFS_DFL_LOG_FACTOR);
} else {
...

E.g., MIN_LOG_BYTES is 10MB and MIN_LOG_BLOCKS is 512 (then multiplied
by 5 here). The latter calculation results in 80MB, so we choose the
former and the subsequent log size validation fails due to not meeting
the minimum block count requirement. It still doesn't make much sense to
me why we use the min here. The minimum log ends up being 16MB for 32k
block size even if we skip the LOG_FACTOR scaling.

Post by Dave Chinner

Post by Brian Foster
well, so I wonder if/how often that format tends to occur.

More often than you think.

Not too surprising. :) FWIW, this is in fact limited to the <16GB fs
case. The small size range probably reduces the chances of hitting this
(as opposed to block size).

Post by Dave Chinner

Post by Brian Foster
What's the situation with regard to >PAGE_SIZE block size support? Is
this something we actually could support today?

Well, the problem is bufferheads and page cache don't support blocks
large than page size. The metadata side of XFS supports it just fine
through the xfs_buf structures, but the file data side doesn't.
That's one of the things I'm slowly trying to find time to fix (i.e.
kill bufferheads).

Ok.

Post by Dave Chinner

Post by Brian Foster
Do we know about any
large page sized arches that could push us into this territory with the
actual page size limitation?

Yes, see above. We have always supported 64k block sizes on Linux
ever since ia64 supported 64k page sizes (i.e. for at least 10
years), so we can't now say "we only support 4k block sizes"....

Indeed, I'd expect to have to support it. I was just looking for more
background.

Post by Dave Chinner

Post by Brian Foster
I suppose if we have >4k page sized arches that utilize block sizes
outside of the 256b-4k range, that's enough to justify the existence of
the range in the general sense. I just might have to factor this area of
code a bit differently. It would also be nice if there was a means to
test.

Grab a ppc64 box from the RH QE guys or ask them to test it....

Will do, thanks.

Brian

Post by Dave Chinner
Cheers,
Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Dave Chinner]

Hi all,
This is a first pass at sparse inode chunk support for XFS. Some
http://oss.sgi.com/archives/xfs/2013-08/msg00346.html
The basic idea is to allow the partial allocation of inode chunks into
fragmented regions of free space. This is accomplished through addition
of a holemask field into the inobt record that defines what portion(s)
of an inode chunk are invalid (i.e., holes in the chunk). This work is
not quite complete, but is at a point where I'd like to start getting
feedback on the design and what direction to take for some of the known
gaps.
- Patches 1-2 - A couple generic cleanups that are dependencies for later
patches in the series.
- Patches 3-5 - Basic data structure update, feature bit and minor
helper introduction.
- Patches 6-7 - Update v5 icreate logging and recovery to handle sparse
inode records.
- Patches 8-13 - Allocation support for sparse inode records. Physical
chunk allocation and individual inode allocation.
- Patches 14-16 - Deallocation support for sparse inode chunks. Physical
chunk deallocation, individual inode free and cluster free.
- Patch 17 - Fixes for bulkstat/inumbers.
- Patch 18 - Activate support for sparse chunk allocation and
processing.
This work is lightly tested for regression (some xfstests failures due
to repair) and basic functionality. I have a new xfstests test I'll
forward along for demonstration purposes.
The current minimum sparse chunk allocation granularity is the cluster
size.

Looking at the patchset (I got to patch 5 that first uses this),
this is problematic. the cluster size is currently a kernel
implementation detail, and not something defined by the on-disk
format. We can change the cluster size in the kernel and not affect
the format on disk. Making the cluster size a part of the disk
format by defining it to be the resolution of sparse inode chunks
changes that - it's now a part of the on-disk inode format, and that
greatly limits what we can do with it.

My initial attempts at this work tried to redefine to the minimum
chunk length based on the holemask granularity (a la the stale macro I
seemingly left in this series ;), but this involves tweaking the
codepaths that use the cluster size (i.e., imap) which proved rather
hairy.

This is where we need to head towards, though. The cluster size is
currently the unit of inode IO, so that needs to be influenced by the
sparse inode chunk granularity. Yes, we can define the inode chunk
granularity to be the same as the cluster size, but that simply
means we need to configure the cluster size appropriately at mount.
It doesn't mean we need to change what cluster size means or it's
implementation....

This also means we need a solution where an imap can change if an
inode was initially mapped as a sparse chunk and said chunk is
subsequently made full. E.g., we'd perhaps need to invalidate the inode
buffers for sparse chunks at the time where they are made full. Given
that, I landed on using the cluster size and leaving those codepaths as
is for the time being.

Again, that's kernel inode buffer cache implementaiton details, not
something that matters for the on-disk format. So really these need
to be separated. Probably means we need a "sparse inode allocation
alignment" field in the superblock to define this. Having
the kernel reject sparse alignments it can't support from the
initial implementation means we can improve the kernel
implementation over time and (eventually) support sub-cluster sized
sparse inode allocation.

i.e. initial implementation only supports sparse alignment ==
cluster size, and rejects everything else....

There is a tradeoff here for v5 superblocks because we've recently made
a change to scale the cluster size based on the factor increase in the
inode size from the default (see xfsprogs commit 7b5f9801). This means
that effectiveness of sparse chunks is tied to whether the level of free
space fragmentation matches the cluster size. By that I mean effectivess
is good (near 100% utilization possible) if free space fragmentation
leaves free extents around that at least match the cluster size. If
fragmentation is worse than the cluster size, effectiveness is reduced.
This can also be demonstrated with the forthcoming xfstests test.

Exactly. We don't need to solve every problem with the initial
implementation - we can iteratively improve the code because once
the fields are one disk we only need to change the kernel
implemenation to support finer grained sparse allocation to solve
this allocation chunk < cluster size problem....

We set an incompatible feature bit once a sparse inode chunk is
allocated because older revisions of code will interpret the non-zero
holemask bits in the higher order bytes of the record freecount. The
feature bit must be removed once all sparse inode chunks are eliminated
one way or another. This series does not currently remove the feature
bit once set simply because I hadn't thought through the mechanism quite
yet. For the next version, I'm thinking about adding an inobt walk
mechanism that can be conditionally invoked (i.e., feature bit is
currently set and a sparse inode chunk has been eliminated) either via
workqueue on an interval or during unmount if necessary. Thoughts or
alternative suggestions on that appreciated.

I wouldn't bother. Let xfs_repair determine if the bit needs to be
set or not when it does it's final superblock write after it has
scanned and repaired the fs.

I'm even in two minds of whether we want the sb bit added
dynamically, because it means the same upgrade/downdgrade cycle can
have different results simply due to filesystem freespace
fragmentation patterns...

Perhaps an xfs-admin command to turn the feature on dynamicallyi for
existing filesystems, kind of like what we did with lazy superblock
counters when they were introduced?

Cheers,

Dave.

[Brian Foster]

Post by Dave Chinner

Hi all,
This is a first pass at sparse inode chunk support for XFS. Some
http://oss.sgi.com/archives/xfs/2013-08/msg00346.html
The basic idea is to allow the partial allocation of inode chunks into
fragmented regions of free space. This is accomplished through addition
of a holemask field into the inobt record that defines what portion(s)
of an inode chunk are invalid (i.e., holes in the chunk). This work is
not quite complete, but is at a point where I'd like to start getting
feedback on the design and what direction to take for some of the known
gaps.
- Patches 1-2 - A couple generic cleanups that are dependencies for later
patches in the series.
- Patches 3-5 - Basic data structure update, feature bit and minor
helper introduction.
- Patches 6-7 - Update v5 icreate logging and recovery to handle sparse
inode records.
- Patches 8-13 - Allocation support for sparse inode records. Physical
chunk allocation and individual inode allocation.
- Patches 14-16 - Deallocation support for sparse inode chunks. Physical
chunk deallocation, individual inode free and cluster free.
- Patch 17 - Fixes for bulkstat/inumbers.
- Patch 18 - Activate support for sparse chunk allocation and
processing.
This work is lightly tested for regression (some xfstests failures due
to repair) and basic functionality. I have a new xfstests test I'll
forward along for demonstration purposes.
The current minimum sparse chunk allocation granularity is the cluster
size.

Looking at the patchset (I got to patch 5 that first uses this),
this is problematic. the cluster size is currently a kernel
implementation detail, and not something defined by the on-disk
format. We can change the cluster size in the kernel and not affect
the format on disk. Making the cluster size a part of the disk
format by defining it to be the resolution of sparse inode chunks
changes that - it's now a part of the on-disk inode format, and that
greatly limits what we can do with it.

I was going off the inoalignmt bit that's set at mkfs time, but looking
at the code I think I see what you mean...

Post by Dave Chinner

My initial attempts at this work tried to redefine to the minimum
chunk length based on the holemask granularity (a la the stale macro I
seemingly left in this series ;), but this involves tweaking the
codepaths that use the cluster size (i.e., imap) which proved rather
hairy.

This is where we need to head towards, though. The cluster size is
currently the unit of inode IO, so that needs to be influenced by the
sparse inode chunk granularity. Yes, we can define the inode chunk
granularity to be the same as the cluster size, but that simply
means we need to configure the cluster size appropriately at mount.
It doesn't mean we need to change what cluster size means or it's
implementation....

I don't think I've necessarily encoded the cluster size into the disk
format explicitly, but rather used it as a heuristic for how to size the
sparse inode chunk allocations. I think what you're saying is that the
cluster size can change across a mount or code update, while the on-disk
allocation state will not. Thus we can't always go on the assumption
that the on-disk allocations will be sane with regard to the current
cluster size. For example, I suppose if the cluster size increased after
we have some existing sparse allocations we'd end up with some broken
buffer mappings for inode chunks.

Post by Dave Chinner

This also means we need a solution where an imap can change if an
inode was initially mapped as a sparse chunk and said chunk is
subsequently made full. E.g., we'd perhaps need to invalidate the inode
buffers for sparse chunks at the time where they are made full. Given
that, I landed on using the cluster size and leaving those codepaths as
is for the time being.

Again, that's kernel inode buffer cache implementaiton details, not
something that matters for the on-disk format. So really these need
to be separated. Probably means we need a "sparse inode allocation
alignment" field in the superblock to define this. Having
the kernel reject sparse alignments it can't support from the
initial implementation means we can improve the kernel
implementation over time and (eventually) support sub-cluster sized
sparse inode allocation.
i.e. initial implementation only supports sparse alignment ==
cluster size, and rejects everything else....

Yeah, that's pretty much what I was trying to accomplish, just not
encoded properly with regard to the superblock options. E.g., I
overloaded the cluster size in a fragile way. Instead, explicitly set
the sparse inode allocation granularity and let that influence the
cluster size. I suspect that means we could even still use the cluster
size heuristic to define the default alloc. granularity, but of course
we aren't married to it so can evaluate that independently. That sounds
much nicer, thanks.

Post by Dave Chinner

There is a tradeoff here for v5 superblocks because we've recently made
a change to scale the cluster size based on the factor increase in the
inode size from the default (see xfsprogs commit 7b5f9801). This means
that effectiveness of sparse chunks is tied to whether the level of free
space fragmentation matches the cluster size. By that I mean effectivess
is good (near 100% utilization possible) if free space fragmentation
leaves free extents around that at least match the cluster size. If
fragmentation is worse than the cluster size, effectiveness is reduced.
This can also be demonstrated with the forthcoming xfstests test.

Exactly. We don't need to solve every problem with the initial
implementation - we can iteratively improve the code because once
the fields are one disk we only need to change the kernel
implemenation to support finer grained sparse allocation to solve
this allocation chunk < cluster size problem....

Right... I didn't want to rule out fixing the imap logic and whatnot at
all by any means. I thought about it a bit, but the solution isn't yet
clear and certainly could involve some more refactoring or rethinking of
abstractions, so I think that's better suited as a follow on effort.

Post by Dave Chinner

We set an incompatible feature bit once a sparse inode chunk is
allocated because older revisions of code will interpret the non-zero
holemask bits in the higher order bytes of the record freecount. The
feature bit must be removed once all sparse inode chunks are eliminated
one way or another. This series does not currently remove the feature
bit once set simply because I hadn't thought through the mechanism quite
yet. For the next version, I'm thinking about adding an inobt walk
mechanism that can be conditionally invoked (i.e., feature bit is
currently set and a sparse inode chunk has been eliminated) either via
workqueue on an interval or during unmount if necessary. Thoughts or
alternative suggestions on that appreciated.

I wouldn't bother. Let xfs_repair determine if the bit needs to be
set or not when it does it's final superblock write after it has
scanned and repaired the fs.

That sounds good to me.

Post by Dave Chinner
I'm even in two minds of whether we want the sb bit added
dynamically, because it means the same upgrade/downdgrade cycle can
have different results simply due to filesystem freespace
fragmentation patterns...
Perhaps an xfs-admin command to turn the feature on dynamicallyi for
existing filesystems, kind of like what we did with lazy superblock
counters when they were introduced?

Hmm, I suppose that does create a new and interesting dynamic with
regard to the feature bit (non-deterministic backwards compatibility).
One could certainly value backwards compatibility over this particular
feature, and there is currently no way to control it. I'll look into
doing something with xfs_admin. In fact, I was thinking of adding
something to tune the cluster size bit to get around the v5 scaling
issue anyways.

Thanks for the feedback. I'm under the weather today so I'll start going
through the rest of it when my head is less foggy.

Brian

Post by Dave Chinner
Cheers,
Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs

[Dave Chinner]

Post by Brian Foster

Post by Dave Chinner

Hi all,
This is a first pass at sparse inode chunk support for XFS. Some
http://oss.sgi.com/archives/xfs/2013-08/msg00346.html
The basic idea is to allow the partial allocation of inode chunks into
fragmented regions of free space. This is accomplished through addition
of a holemask field into the inobt record that defines what portion(s)
of an inode chunk are invalid (i.e., holes in the chunk). This work is
not quite complete, but is at a point where I'd like to start getting
feedback on the design and what direction to take for some of the known
gaps.
- Patches 1-2 - A couple generic cleanups that are dependencies for later
patches in the series.
- Patches 3-5 - Basic data structure update, feature bit and minor
helper introduction.
- Patches 6-7 - Update v5 icreate logging and recovery to handle sparse
inode records.
- Patches 8-13 - Allocation support for sparse inode records. Physical
chunk allocation and individual inode allocation.
- Patches 14-16 - Deallocation support for sparse inode chunks. Physical
chunk deallocation, individual inode free and cluster free.
- Patch 17 - Fixes for bulkstat/inumbers.
- Patch 18 - Activate support for sparse chunk allocation and
processing.
This work is lightly tested for regression (some xfstests failures due
to repair) and basic functionality. I have a new xfstests test I'll
forward along for demonstration purposes.
The current minimum sparse chunk allocation granularity is the cluster
size.

Looking at the patchset (I got to patch 5 that first uses this),
this is problematic. the cluster size is currently a kernel
implementation detail, and not something defined by the on-disk
format. We can change the cluster size in the kernel and not affect
the format on disk. Making the cluster size a part of the disk
format by defining it to be the resolution of sparse inode chunks
changes that - it's now a part of the on-disk inode format, and that
greatly limits what we can do with it.

I was going off the inoalignmt bit that's set at mkfs time, but looking
at the code I think I see what you mean...

Post by Dave Chinner

My initial attempts at this work tried to redefine to the minimum
chunk length based on the holemask granularity (a la the stale macro I
seemingly left in this series ;), but this involves tweaking the
codepaths that use the cluster size (i.e., imap) which proved rather
hairy.

This is where we need to head towards, though. The cluster size is
currently the unit of inode IO, so that needs to be influenced by the
sparse inode chunk granularity. Yes, we can define the inode chunk
granularity to be the same as the cluster size, but that simply
means we need to configure the cluster size appropriately at mount.
It doesn't mean we need to change what cluster size means or it's
implementation....

I don't think I've necessarily encoded the cluster size into the disk
format explicitly, but rather used it as a heuristic for how to size the
sparse inode chunk allocations. I think what you're saying is that the
cluster size can change across a mount or code update, while the on-disk
allocation state will not. Thus we can't always go on the assumption
that the on-disk allocations will be sane with regard to the current
cluster size. For example, I suppose if the cluster size increased after
we have some existing sparse allocations we'd end up with some broken
buffer mappings for inode chunks.

Yup, that's the problem in a nutshell: cluster size is not fixed.

Post by Brian Foster

Post by Dave Chinner
Again, that's kernel inode buffer cache implementaiton details, not
something that matters for the on-disk format. So really these need
to be separated. Probably means we need a "sparse inode allocation
alignment" field in the superblock to define this. Having
the kernel reject sparse alignments it can't support from the
initial implementation means we can improve the kernel
implementation over time and (eventually) support sub-cluster sized
sparse inode allocation.
i.e. initial implementation only supports sparse alignment ==
cluster size, and rejects everything else....

Yeah, that's pretty much what I was trying to accomplish, just not
encoded properly with regard to the superblock options. E.g., I
overloaded the cluster size in a fragile way. Instead, explicitly set
the sparse inode allocation granularity and let that influence the
cluster size. I suspect that means we could even still use the cluster
size heuristic to define the default alloc. granularity, but of course
we aren't married to it so can evaluate that independently. That sounds
much nicer, thanks.

Right. We may not do anything else in the short or medium term, but
having a separate superblock field gives us the flexibility for
change the inode cluster implementation/behaviour in the future...

Post by Brian Foster

Post by Dave Chinner
dynamically, because it means the same upgrade/downdgrade cycle can
have different results simply due to filesystem freespace
fragmentation patterns...
Perhaps an xfs-admin command to turn the feature on dynamicallyi for
existing filesystems, kind of like what we did with lazy superblock
counters when they were introduced?

Hmm, I suppose that does create a new and interesting dynamic with
regard to the feature bit (non-deterministic backwards compatibility).
One could certainly value backwards compatibility over this particular
feature, and there is currently no way to control it. I'll look into
doing something with xfs_admin. In fact, I was thinking of adding
something to tune the cluster size bit to get around the v5 scaling
issue anyways.

What v5 scalability issue is that? I don't recall any outstanding
issues with inode cluster IO....

Cheers,

Dave.

[Brian Foster]

Post by Dave Chinner

Post by Brian Foster

Post by Dave Chinner

Hi all,
This is a first pass at sparse inode chunk support for XFS. Some
http://oss.sgi.com/archives/xfs/2013-08/msg00346.html
The basic idea is to allow the partial allocation of inode chunks into
fragmented regions of free space. This is accomplished through addition
of a holemask field into the inobt record that defines what portion(s)
of an inode chunk are invalid (i.e., holes in the chunk). This work is
not quite complete, but is at a point where I'd like to start getting
feedback on the design and what direction to take for some of the known
gaps.
- Patches 1-2 - A couple generic cleanups that are dependencies for later
patches in the series.
- Patches 3-5 - Basic data structure update, feature bit and minor
helper introduction.
- Patches 6-7 - Update v5 icreate logging and recovery to handle sparse
inode records.
- Patches 8-13 - Allocation support for sparse inode records. Physical
chunk allocation and individual inode allocation.
- Patches 14-16 - Deallocation support for sparse inode chunks. Physical
chunk deallocation, individual inode free and cluster free.
- Patch 17 - Fixes for bulkstat/inumbers.
- Patch 18 - Activate support for sparse chunk allocation and
processing.
This work is lightly tested for regression (some xfstests failures due
to repair) and basic functionality. I have a new xfstests test I'll
forward along for demonstration purposes.
The current minimum sparse chunk allocation granularity is the cluster
size.

Looking at the patchset (I got to patch 5 that first uses this),
this is problematic. the cluster size is currently a kernel
implementation detail, and not something defined by the on-disk
format. We can change the cluster size in the kernel and not affect
the format on disk. Making the cluster size a part of the disk
format by defining it to be the resolution of sparse inode chunks
changes that - it's now a part of the on-disk inode format, and that
greatly limits what we can do with it.

I was going off the inoalignmt bit that's set at mkfs time, but looking
at the code I think I see what you mean...

Post by Dave Chinner

My initial attempts at this work tried to redefine to the minimum
chunk length based on the holemask granularity (a la the stale macro I
seemingly left in this series ;), but this involves tweaking the
codepaths that use the cluster size (i.e., imap) which proved rather
hairy.

This is where we need to head towards, though. The cluster size is
currently the unit of inode IO, so that needs to be influenced by the
sparse inode chunk granularity. Yes, we can define the inode chunk
granularity to be the same as the cluster size, but that simply
means we need to configure the cluster size appropriately at mount.
It doesn't mean we need to change what cluster size means or it's
implementation....

I don't think I've necessarily encoded the cluster size into the disk
format explicitly, but rather used it as a heuristic for how to size the
sparse inode chunk allocations. I think what you're saying is that the
cluster size can change across a mount or code update, while the on-disk
allocation state will not. Thus we can't always go on the assumption
that the on-disk allocations will be sane with regard to the current
cluster size. For example, I suppose if the cluster size increased after
we have some existing sparse allocations we'd end up with some broken
buffer mappings for inode chunks.

Yup, that's the problem in a nutshell: cluster size is not fixed.

Post by Brian Foster

Post by Dave Chinner
Again, that's kernel inode buffer cache implementaiton details, not
something that matters for the on-disk format. So really these need
to be separated. Probably means we need a "sparse inode allocation
alignment" field in the superblock to define this. Having
the kernel reject sparse alignments it can't support from the
initial implementation means we can improve the kernel
implementation over time and (eventually) support sub-cluster sized
sparse inode allocation.
i.e. initial implementation only supports sparse alignment ==
cluster size, and rejects everything else....

Yeah, that's pretty much what I was trying to accomplish, just not
encoded properly with regard to the superblock options. E.g., I
overloaded the cluster size in a fragile way. Instead, explicitly set
the sparse inode allocation granularity and let that influence the
cluster size. I suspect that means we could even still use the cluster
size heuristic to define the default alloc. granularity, but of course
we aren't married to it so can evaluate that independently. That sounds
much nicer, thanks.

Right. We may not do anything else in the short or medium term, but
having a separate superblock field gives us the flexibility for
change the inode cluster implementation/behaviour in the future...

Post by Brian Foster

Post by Dave Chinner
dynamically, because it means the same upgrade/downdgrade cycle can
have different results simply due to filesystem freespace
fragmentation patterns...
Perhaps an xfs-admin command to turn the feature on dynamicallyi for
existing filesystems, kind of like what we did with lazy superblock
counters when they were introduced?

Hmm, I suppose that does create a new and interesting dynamic with
regard to the feature bit (non-deterministic backwards compatibility).
One could certainly value backwards compatibility over this particular
feature, and there is currently no way to control it. I'll look into
doing something with xfs_admin. In fact, I was thinking of adding
something to tune the cluster size bit to get around the v5 scaling
issue anyways.

What v5 scalability issue is that? I don't recall any outstanding
issues with inode cluster IO....

There's no scalability issue... I'm just referring to the fact that we
scale the cluster size by the inode size increase factor on v5
superblocks.

E.g., my free space fragmentation xfstests test started out with a fixed
file size based on something close to the worst case with an
implementation that used the allocation granularity of max(<holemask bit
granularity>, <inodes per block>). Once I tied the implementation to the
cluster size due to the aforementioned complexities, it became apparent
the test was less effective with my chosen file size on v5 supers,
particularly as the inode size increased. So from there I was
considering a similar xfs_admin command a user could run to reduce the
cluster size as a backstop should this limitation arise in the real
world. We can start with doing something just to enable the feature as
outlined above and revisit this then...

Brian

Post by Dave Chinner
Cheers,
Dave.
--
Dave Chinner

[Dave Chinner]

Post by Brian Foster

Post by Dave Chinner

Post by Brian Foster
Hmm, I suppose that does create a new and interesting dynamic with
regard to the feature bit (non-deterministic backwards compatibility).
One could certainly value backwards compatibility over this particular
feature, and there is currently no way to control it. I'll look into
doing something with xfs_admin. In fact, I was thinking of adding
something to tune the cluster size bit to get around the v5 scaling
issue anyways.

What v5 scalability issue is that? I don't recall any outstanding
issues with inode cluster IO....

There's no scalability issue... I'm just referring to the fact that we
scale the cluster size by the inode size increase factor on v5
superblocks.
E.g., my free space fragmentation xfstests test started out with a fixed
file size based on something close to the worst case with an
implementation that used the allocation granularity of max(<holemask bit
granularity>, <inodes per block>). Once I tied the implementation to the
cluster size due to the aforementioned complexities, it became apparent
the test was less effective with my chosen file size on v5 supers,
particularly as the inode size increased. So from there I was
considering a similar xfs_admin command a user could run to reduce the
cluster size as a backstop should this limitation arise in the real
world. We can start with doing something just to enable the feature as
outlined above and revisit this then...

Right, but I'd suggest that the better long term solution to avoid
the limitations of inode cluster buffer alignment issues is to get
rid of inode clusters and inode buffers altogether. We only need
inode buffers for logging unlinked list modifications, so once we
log those as part of the inode core for for v5 filesystems then we
can do much more dynamic inode IO. That then frees us up to do fine
grained sparse inode allocation because we aren't limited by
in-memory buffering limitations.

http://xfs.org/index.php/Improving_inode_Caching#Food_For_Thought_.28Crazy_Ideas.29

Cheers,

Dave.

[Brian Foster]

Post by Dave Chinner

Post by Brian Foster

Post by Dave Chinner

Post by Brian Foster
Hmm, I suppose that does create a new and interesting dynamic with
regard to the feature bit (non-deterministic backwards compatibility).
One could certainly value backwards compatibility over this particular
feature, and there is currently no way to control it. I'll look into
doing something with xfs_admin. In fact, I was thinking of adding
something to tune the cluster size bit to get around the v5 scaling
issue anyways.

What v5 scalability issue is that? I don't recall any outstanding
issues with inode cluster IO....

There's no scalability issue... I'm just referring to the fact that we
scale the cluster size by the inode size increase factor on v5
superblocks.
E.g., my free space fragmentation xfstests test started out with a fixed
file size based on something close to the worst case with an
implementation that used the allocation granularity of max(<holemask bit
granularity>, <inodes per block>). Once I tied the implementation to the
cluster size due to the aforementioned complexities, it became apparent
the test was less effective with my chosen file size on v5 supers,
particularly as the inode size increased. So from there I was
considering a similar xfs_admin command a user could run to reduce the
cluster size as a backstop should this limitation arise in the real
world. We can start with doing something just to enable the feature as
outlined above and revisit this then...

Right, but I'd suggest that the better long term solution to avoid
the limitations of inode cluster buffer alignment issues is to get
rid of inode clusters and inode buffers altogether. We only need
inode buffers for logging unlinked list modifications, so once we
log those as part of the inode core for for v5 filesystems then we
can do much more dynamic inode IO. That then frees us up to do fine
grained sparse inode allocation because we aren't limited by
in-memory buffering limitations.
http://xfs.org/index.php/Improving_inode_Caching#Food_For_Thought_.28Crazy_Ideas.29

Interesting, thanks. I guess removing the need for the code that's
incompatible with the sub-cluster size granularity is a nice option. :)
I'll have to read into this some more once the basic sparse inode
feature is more hashed out.

Brian

Post by Dave Chinner
Cheers,
Dave.
--
Dave Chinner
_______________________________________________
xfs mailing list
http://oss.sgi.com/mailman/listinfo/xfs