| Message ID | 20231126130124.1251467-4-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/4] xfs: clean up the XFS_IOC_{GS}ET_RESBLKS handler | expand |
On Sun, Nov 26, 2023 at 02:01:23PM +0100, Christoph Hellwig wrote: > xfs_reserve_blocks has a very odd interface that can only be explained > by it directly deriving from the IRIX fcntl handler back in the day. > > Split reporting out the reserved blocks out of xfs_reserve_blocks into > the only caller that cares. This means that the value reported from > XFS_IOC_SET_RESBLKS isn't atomically sampled in the same critical > section as when it was set anymore, but as the values could change That wasn't true for the case of increasing the reserve before this patch either. :) > right after setting them anyway that does not matter. It does > provide atomic sampling of both values for XFS_IOC_GET_RESBLKS now, > though. Hey, that's neat! > Also pass a normal scalar integer value for the requested value instead > of the pointless pointer. Guh. What a calling convention. > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_fsops.c | 34 +++------------------------------- > fs/xfs/xfs_fsops.h | 3 +-- > fs/xfs/xfs_ioctl.c | 13 ++++++------- > fs/xfs/xfs_mount.c | 8 ++------ > fs/xfs/xfs_super.c | 6 ++---- > 5 files changed, 14 insertions(+), 50 deletions(-) > > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > index 01681783e2c31a..4f5da19142f298 100644 > --- a/fs/xfs/xfs_fsops.c > +++ b/fs/xfs/xfs_fsops.c > @@ -344,43 +344,20 @@ xfs_growfs_log( > } > > /* > - * exported through ioctl XFS_IOC_SET_RESBLKS & XFS_IOC_GET_RESBLKS > - * > - * xfs_reserve_blocks is called to set m_resblks > - * in the in-core mount table. The number of unused reserved blocks > - * is kept in m_resblks_avail. > - * > * Reserve the requested number of blocks if available. Otherwise return > * as many as possible to satisfy the request. The actual number > - * reserved are returned in outval > - * > - * A null inval pointer indicates that only the current reserved blocks > - * available should be returned no settings are changed. > + * reserved are returned in outval. > */ > - > int > xfs_reserve_blocks( > - xfs_mount_t *mp, > - uint64_t *inval, > - xfs_fsop_resblks_t *outval) > + struct xfs_mount *mp, > + uint64_t request) > { > int64_t lcounter, delta; > int64_t fdblks_delta = 0; > - uint64_t request; > int64_t free; > int error = 0; > > - /* If inval is null, report current values and return */ > - if (inval == (uint64_t *)NULL) { > - if (!outval) > - return -EINVAL; > - outval->resblks = mp->m_resblks; > - outval->resblks_avail = mp->m_resblks_avail; > - return 0; > - } > - > - request = *inval; > - > /* > * With per-cpu counters, this becomes an interesting problem. we need > * to work out if we are freeing or allocation blocks first, then we can > @@ -450,11 +427,6 @@ xfs_reserve_blocks( > spin_lock(&mp->m_sb_lock); > } > out: > - if (outval) { > - outval->resblks = mp->m_resblks; > - outval->resblks_avail = mp->m_resblks_avail; > - } > - > spin_unlock(&mp->m_sb_lock); > return error; > } > diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h > index 45f0cb6e805938..7536f8a92746f6 100644 > --- a/fs/xfs/xfs_fsops.h > +++ b/fs/xfs/xfs_fsops.h > @@ -8,8 +8,7 @@ > > extern int xfs_growfs_data(struct xfs_mount *mp, struct xfs_growfs_data *in); > extern int xfs_growfs_log(struct xfs_mount *mp, struct xfs_growfs_log *in); > -extern int xfs_reserve_blocks(xfs_mount_t *mp, uint64_t *inval, > - xfs_fsop_resblks_t *outval); > +int xfs_reserve_blocks(struct xfs_mount *mp, uint64_t request); > extern int xfs_fs_goingdown(xfs_mount_t *mp, uint32_t inflags); > > extern int xfs_fs_reserve_ag_blocks(struct xfs_mount *mp); > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index c8e78c8101c65c..812efb7923abb1 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -1871,7 +1871,6 @@ xfs_ioctl_getset_resblocks( > struct xfs_mount *mp = XFS_I(file_inode(filp))->i_mount; > struct xfs_fsop_resblks fsop = { }; > int error; > - uint64_t in; > > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > @@ -1886,17 +1885,17 @@ xfs_ioctl_getset_resblocks( > error = mnt_want_write_file(filp); > if (error) > return error; > - in = fsop.resblks; > - error = xfs_reserve_blocks(mp, &in, &fsop); > + error = xfs_reserve_blocks(mp, fsop.resblks); > mnt_drop_write_file(filp); > if (error) > return error; > - } else { > - error = xfs_reserve_blocks(mp, NULL, &fsop); > - if (error) > - return error; > } > > + spin_lock(&mp->m_sb_lock); > + fsop.resblks = mp->m_resblks; > + fsop.resblks_avail = mp->m_resblks_avail; > + spin_unlock(&mp->m_sb_lock); Hm. I sorta preferred keeping these details hidden in xfs_fsops.c rather than scattering them around and lengthening xfs_ioctl.c, but I think the calling convention cleanup is worthy enough for: Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + > if (copy_to_user(arg, &fsop, sizeof(fsop))) > return -EFAULT; > return 0; > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c > index aed5be5508fe57..aabb25dc3efab2 100644 > --- a/fs/xfs/xfs_mount.c > +++ b/fs/xfs/xfs_mount.c > @@ -637,7 +637,6 @@ xfs_mountfs( > struct xfs_sb *sbp = &(mp->m_sb); > struct xfs_inode *rip; > struct xfs_ino_geometry *igeo = M_IGEO(mp); > - uint64_t resblks; > uint quotamount = 0; > uint quotaflags = 0; > int error = 0; > @@ -974,8 +973,7 @@ xfs_mountfs( > * we were already there on the last unmount. Warn if this occurs. > */ > if (!xfs_is_readonly(mp)) { > - resblks = xfs_default_resblks(mp); > - error = xfs_reserve_blocks(mp, &resblks, NULL); > + error = xfs_reserve_blocks(mp, xfs_default_resblks(mp)); > if (error) > xfs_warn(mp, > "Unable to allocate reserve blocks. Continuing without reserve pool."); > @@ -1053,7 +1051,6 @@ void > xfs_unmountfs( > struct xfs_mount *mp) > { > - uint64_t resblks; > int error; > > /* > @@ -1090,8 +1087,7 @@ xfs_unmountfs( > * we only every apply deltas to the superblock and hence the incore > * value does not matter.... > */ > - resblks = 0; > - error = xfs_reserve_blocks(mp, &resblks, NULL); > + error = xfs_reserve_blocks(mp, 0); > if (error) > xfs_warn(mp, "Unable to free reserved block pool. " > "Freespace may not be correct on next mount."); > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 764304595e8b00..d0009430a62778 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -906,10 +906,8 @@ xfs_fs_statfs( > STATIC void > xfs_save_resvblks(struct xfs_mount *mp) > { > - uint64_t resblks = 0; > - > mp->m_resblks_save = mp->m_resblks; > - xfs_reserve_blocks(mp, &resblks, NULL); > + xfs_reserve_blocks(mp, 0); > } > > STATIC void > @@ -923,7 +921,7 @@ xfs_restore_resvblks(struct xfs_mount *mp) > } else > resblks = xfs_default_resblks(mp); > > - xfs_reserve_blocks(mp, &resblks, NULL); > + xfs_reserve_blocks(mp, resblks); > } > > /* > -- > 2.39.2 > >
On Mon, Nov 27, 2023 at 06:09:33PM -0800, Darrick J. Wong wrote: > > + spin_lock(&mp->m_sb_lock); > > + fsop.resblks = mp->m_resblks; > > + fsop.resblks_avail = mp->m_resblks_avail; > > + spin_unlock(&mp->m_sb_lock); > > Hm. I sorta preferred keeping these details hidden in xfs_fsops.c > rather than scattering them around and lengthening xfs_ioctl.c, but > I think the calling convention cleanup is worthy enough for: If you prefer I can keep a helper to fill in a xfs_fsop_resblks structure under m_sb_lock in fsops.c, but I'm not sure that's worth it.
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c index 01681783e2c31a..4f5da19142f298 100644 --- a/fs/xfs/xfs_fsops.c +++ b/fs/xfs/xfs_fsops.c @@ -344,43 +344,20 @@ xfs_growfs_log( } /* - * exported through ioctl XFS_IOC_SET_RESBLKS & XFS_IOC_GET_RESBLKS - * - * xfs_reserve_blocks is called to set m_resblks - * in the in-core mount table. The number of unused reserved blocks - * is kept in m_resblks_avail. - * * Reserve the requested number of blocks if available. Otherwise return * as many as possible to satisfy the request. The actual number - * reserved are returned in outval - * - * A null inval pointer indicates that only the current reserved blocks - * available should be returned no settings are changed. + * reserved are returned in outval. */ - int xfs_reserve_blocks( - xfs_mount_t *mp, - uint64_t *inval, - xfs_fsop_resblks_t *outval) + struct xfs_mount *mp, + uint64_t request) { int64_t lcounter, delta; int64_t fdblks_delta = 0; - uint64_t request; int64_t free; int error = 0; - /* If inval is null, report current values and return */ - if (inval == (uint64_t *)NULL) { - if (!outval) - return -EINVAL; - outval->resblks = mp->m_resblks; - outval->resblks_avail = mp->m_resblks_avail; - return 0; - } - - request = *inval; - /* * With per-cpu counters, this becomes an interesting problem. we need * to work out if we are freeing or allocation blocks first, then we can @@ -450,11 +427,6 @@ xfs_reserve_blocks( spin_lock(&mp->m_sb_lock); } out: - if (outval) { - outval->resblks = mp->m_resblks; - outval->resblks_avail = mp->m_resblks_avail; - } - spin_unlock(&mp->m_sb_lock); return error; } diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h index 45f0cb6e805938..7536f8a92746f6 100644 --- a/fs/xfs/xfs_fsops.h +++ b/fs/xfs/xfs_fsops.h @@ -8,8 +8,7 @@ extern int xfs_growfs_data(struct xfs_mount *mp, struct xfs_growfs_data *in); extern int xfs_growfs_log(struct xfs_mount *mp, struct xfs_growfs_log *in); -extern int xfs_reserve_blocks(xfs_mount_t *mp, uint64_t *inval, - xfs_fsop_resblks_t *outval); +int xfs_reserve_blocks(struct xfs_mount *mp, uint64_t request); extern int xfs_fs_goingdown(xfs_mount_t *mp, uint32_t inflags); extern int xfs_fs_reserve_ag_blocks(struct xfs_mount *mp); diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index c8e78c8101c65c..812efb7923abb1 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -1871,7 +1871,6 @@ xfs_ioctl_getset_resblocks( struct xfs_mount *mp = XFS_I(file_inode(filp))->i_mount; struct xfs_fsop_resblks fsop = { }; int error; - uint64_t in; if (!capable(CAP_SYS_ADMIN)) return -EPERM; @@ -1886,17 +1885,17 @@ xfs_ioctl_getset_resblocks( error = mnt_want_write_file(filp); if (error) return error; - in = fsop.resblks; - error = xfs_reserve_blocks(mp, &in, &fsop); + error = xfs_reserve_blocks(mp, fsop.resblks); mnt_drop_write_file(filp); if (error) return error; - } else { - error = xfs_reserve_blocks(mp, NULL, &fsop); - if (error) - return error; } + spin_lock(&mp->m_sb_lock); + fsop.resblks = mp->m_resblks; + fsop.resblks_avail = mp->m_resblks_avail; + spin_unlock(&mp->m_sb_lock); + if (copy_to_user(arg, &fsop, sizeof(fsop))) return -EFAULT; return 0; diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c index aed5be5508fe57..aabb25dc3efab2 100644 --- a/fs/xfs/xfs_mount.c +++ b/fs/xfs/xfs_mount.c @@ -637,7 +637,6 @@ xfs_mountfs( struct xfs_sb *sbp = &(mp->m_sb); struct xfs_inode *rip; struct xfs_ino_geometry *igeo = M_IGEO(mp); - uint64_t resblks; uint quotamount = 0; uint quotaflags = 0; int error = 0; @@ -974,8 +973,7 @@ xfs_mountfs( * we were already there on the last unmount. Warn if this occurs. */ if (!xfs_is_readonly(mp)) { - resblks = xfs_default_resblks(mp); - error = xfs_reserve_blocks(mp, &resblks, NULL); + error = xfs_reserve_blocks(mp, xfs_default_resblks(mp)); if (error) xfs_warn(mp, "Unable to allocate reserve blocks. Continuing without reserve pool."); @@ -1053,7 +1051,6 @@ void xfs_unmountfs( struct xfs_mount *mp) { - uint64_t resblks; int error; /* @@ -1090,8 +1087,7 @@ xfs_unmountfs( * we only every apply deltas to the superblock and hence the incore * value does not matter.... */ - resblks = 0; - error = xfs_reserve_blocks(mp, &resblks, NULL); + error = xfs_reserve_blocks(mp, 0); if (error) xfs_warn(mp, "Unable to free reserved block pool. " "Freespace may not be correct on next mount."); diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 764304595e8b00..d0009430a62778 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -906,10 +906,8 @@ xfs_fs_statfs( STATIC void xfs_save_resvblks(struct xfs_mount *mp) { - uint64_t resblks = 0; - mp->m_resblks_save = mp->m_resblks; - xfs_reserve_blocks(mp, &resblks, NULL); + xfs_reserve_blocks(mp, 0); } STATIC void @@ -923,7 +921,7 @@ xfs_restore_resvblks(struct xfs_mount *mp) } else resblks = xfs_default_resblks(mp); - xfs_reserve_blocks(mp, &resblks, NULL); + xfs_reserve_blocks(mp, resblks); } /*
xfs_reserve_blocks has a very odd interface that can only be explained by it directly deriving from the IRIX fcntl handler back in the day. Split reporting out the reserved blocks out of xfs_reserve_blocks into the only caller that cares. This means that the value reported from XFS_IOC_SET_RESBLKS isn't atomically sampled in the same critical section as when it was set anymore, but as the values could change right after setting them anyway that does not matter. It does provide atomic sampling of both values for XFS_IOC_GET_RESBLKS now, though. Also pass a normal scalar integer value for the requested value instead of the pointless pointer. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_fsops.c | 34 +++------------------------------- fs/xfs/xfs_fsops.h | 3 +-- fs/xfs/xfs_ioctl.c | 13 ++++++------- fs/xfs/xfs_mount.c | 8 ++------ fs/xfs/xfs_super.c | 6 ++---- 5 files changed, 14 insertions(+), 50 deletions(-)