On Mon, Dec 02, 2019 at 06:30:41PM -0800, Darrick J. Wong wrote: > On Tue, Dec 03, 2019 at 08:21:40AM +1100, Dave Chinner wrote: > > On Mon, Dec 02, 2019 at 09:35:38AM -0800, Darrick J. Wong wrote: > > > diff --git a/fs/xfs/libxfs/xfs_ialloc.h b/fs/xfs/libxfs/xfs_ialloc.h > > > index 323592d563d5..9d9fe7b488b8 100644 > > > --- a/fs/xfs/libxfs/xfs_ialloc.h > > > +++ b/fs/xfs/libxfs/xfs_ialloc.h > > > @@ -152,5 +152,7 @@ int xfs_inobt_insert_rec(struct xfs_btree_cur *cur, uint16_t holemask, > > > > > > int xfs_ialloc_cluster_alignment(struct xfs_mount *mp); > > > void xfs_ialloc_setup_geometry(struct xfs_mount *mp); > > > +void xfs_ialloc_find_prealloc(struct xfs_mount *mp, xfs_agino_t *first_agino, > > > + xfs_agino_t *last_agino); > > > > > > #endif /* __XFS_IALLOC_H__ */ > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > > index 7b35d62ede9f..d830a9e13817 100644 > > > --- a/fs/xfs/xfs_ioctl.c > > > +++ b/fs/xfs/xfs_ioctl.c > > > @@ -891,6 +891,9 @@ xfs_ioc_fsgeometry( > > > > > > xfs_fs_geometry(&mp->m_sb, &fsgeo, struct_version); > > > > > > + fsgeo.sunit = mp->m_sb.sb_unit; > > > + fsgeo.swidth = mp->m_sb.sb_width; > > > > Why? > > This was in keeping with Alex' suggestion to use the sunit values incore > even if we don't update the superblock. Not sure about that. If we are getting the geometry for the purposes of working out where something is on disk (e.g. the root inode :), then we need what is in the superblock, not what is in memory... > > > + if (sbp->sb_unit == mp->m_dalign && > > > + sbp->sb_width == mp->m_swidth) > > > + return 0; > > > + > > > + old_su = sbp->sb_unit; > > > + old_sw = sbp->sb_width; > > > + sbp->sb_unit = mp->m_dalign; > > > + sbp->sb_width = mp->m_swidth; > > > + xfs_ialloc_find_prealloc(mp, &first, &last); > > > > We just chuck last away? why calculate it then? > > Hmmm. Repair uses it to silence the "inode chunk claims used block" > error if an inobt record points to something owned by XR_E_INUSE_FS* if > the inode points to something in that first chunk. Not sure /why/ it > does that; it seems to have done that since the creation of the git > repo. Hysterical raisins that have long since decomposed, I'm guessing.... > Frankly, I'm not convinced that's the right behavior; the root inode > chunk should never collide with something else, period. *nod* I suspect the way repair uses the last_prealloc_ino can go away, especially as the inode number calculated is not correct in the first place... > > And why not just > > pass mp->m_dalign/mp->m_swidth into the function rather than setting > > them in the sb and then having to undo the change? i.e. > > > > rootino = xfs_ialloc_calc_rootino(mp, mp->m_dalign, mp->m_swidth); > > <shrug> The whole point was to create a function that computes where the > first allocated inode chunk should be from an existing mountpoint and > superblock, maybe the caller should make a copy, update the parameters, > and then pass the copy into this function? That's a whole lot of cruft that we can avoid just by passing in our specific stripe alignment. What we need to kow is whether a specific stripe geometry will result in the root inode location changing, and so I'm of the opinion we should just write a function that calculates the location based on the supplied geometry and the caller can do whatever checks it needs to with the inode number returned. That provides what both repair and the kernel mount validation requires... > > Should this also return EINVAL, as per above when the DALIGN sb > > feature bit is not set? > > I dunno. We've never rejected these mount options before, which makes > me a little hesitant to break everybody's scripts, even if it /is/ > improper behavior that leads to repair failure. We /do/ have the option > that Alex suggested of modifying the incore values to change the > allocator behavior without committing them to the superblock, which is > what this patch does. > > OTOH the manual pages say that you're not supposed to do this, which > might be a strong enough reason to start banning it. > > Thoughts? On second thoughts, knowing that many users have put sunit/swidth in their fstab, we probably shouldn't make it return an error as that may make their systems unbootable. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
