| Message ID | 20210108190919.623672-3-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: support shrinking free space in the last AG | expand |
On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote: > Such usage isn't encouraged by the kernel coding style. > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > --- > fs/xfs/libxfs/xfs_fs.h | 4 ++-- > fs/xfs/xfs_fsops.c | 12 ++++++------ > fs/xfs/xfs_fsops.h | 4 ++-- > fs/xfs/xfs_ioctl.c | 4 ++-- > 4 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > index 2a2e3cfd94f0..a17313efc1fe 100644 > --- a/fs/xfs/libxfs/xfs_fs.h > +++ b/fs/xfs/libxfs/xfs_fs.h > @@ -308,12 +308,12 @@ struct xfs_ag_geometry { > typedef struct xfs_growfs_data { > __u64 newblocks; /* new data subvol size, fsblocks */ > __u32 imaxpct; /* new inode space percentage limit */ > -} xfs_growfs_data_t; > +}; So long as Eric is ok with fixing this up in xfs_fs_compat.h in userspace, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > > typedef struct xfs_growfs_log { > __u32 newblocks; /* new log size, fsblocks */ > __u32 isint; /* 1 if new log is internal */ > -} xfs_growfs_log_t; > +}; > > typedef struct xfs_growfs_rt { > __u64 newblocks; /* new realtime size, fsblocks */ > diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c > index d254588f6e21..6c5f6a50da2e 100644 > --- a/fs/xfs/xfs_fsops.c > +++ b/fs/xfs/xfs_fsops.c > @@ -25,8 +25,8 @@ > */ > static int > xfs_growfs_data_private( > - xfs_mount_t *mp, /* mount point for filesystem */ > - xfs_growfs_data_t *in) /* growfs data input struct */ > + struct xfs_mount *mp, /* mount point for filesystem */ > + struct xfs_growfs_data *in) /* growfs data input struct */ > { > struct xfs_buf *bp; > int error; > @@ -35,7 +35,7 @@ xfs_growfs_data_private( > xfs_rfsblock_t nb, nb_mod; > xfs_rfsblock_t delta; > xfs_agnumber_t oagcount; > - xfs_trans_t *tp; > + struct xfs_trans *tp; > struct aghdr_init_data id = {}; > > nb = in->newblocks; > @@ -170,8 +170,8 @@ xfs_growfs_data_private( > > static int > xfs_growfs_log_private( > - xfs_mount_t *mp, /* mount point for filesystem */ > - xfs_growfs_log_t *in) /* growfs log input struct */ > + struct xfs_mount *mp, /* mount point for filesystem */ > + struct xfs_growfs_log *in) /* growfs log input struct */ > { > xfs_extlen_t nb; > > @@ -268,7 +268,7 @@ xfs_growfs_data( > int > xfs_growfs_log( > xfs_mount_t *mp, > - xfs_growfs_log_t *in) > + struct xfs_growfs_log *in) > { > int error; > > diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h > index 92869f6ec8d3..d7e9af4a28eb 100644 > --- a/fs/xfs/xfs_fsops.h > +++ b/fs/xfs/xfs_fsops.h > @@ -6,8 +6,8 @@ > #ifndef __XFS_FSOPS_H__ > #define __XFS_FSOPS_H__ > > -extern int xfs_growfs_data(xfs_mount_t *mp, xfs_growfs_data_t *in); > -extern int xfs_growfs_log(xfs_mount_t *mp, xfs_growfs_log_t *in); > +extern int xfs_growfs_data(xfs_mount_t *mp, struct xfs_growfs_data *in); > +extern int xfs_growfs_log(xfs_mount_t *mp, struct xfs_growfs_log *in); > extern void xfs_fs_counts(xfs_mount_t *mp, xfs_fsop_counts_t *cnt); > extern int xfs_reserve_blocks(xfs_mount_t *mp, uint64_t *inval, > xfs_fsop_resblks_t *outval); > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 3fbd98f61ea5..a62520f49ec5 100644 > --- a/fs/xfs/xfs_ioctl.c > +++ b/fs/xfs/xfs_ioctl.c > @@ -2260,7 +2260,7 @@ xfs_file_ioctl( > } > > case XFS_IOC_FSGROWFSDATA: { > - xfs_growfs_data_t in; > + struct xfs_growfs_data in; > > if (copy_from_user(&in, arg, sizeof(in))) > return -EFAULT; > @@ -2274,7 +2274,7 @@ xfs_file_ioctl( > } > > case XFS_IOC_FSGROWFSLOG: { > - xfs_growfs_log_t in; > + struct xfs_growfs_log in; > > if (copy_from_user(&in, arg, sizeof(in))) > return -EFAULT; > -- > 2.27.0 >
On 1/8/21 3:21 PM, Darrick J. Wong wrote: > On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote: >> Such usage isn't encouraged by the kernel coding style. >> >> Signed-off-by: Gao Xiang <hsiangkao@redhat.com> >> --- >> fs/xfs/libxfs/xfs_fs.h | 4 ++-- >> fs/xfs/xfs_fsops.c | 12 ++++++------ >> fs/xfs/xfs_fsops.h | 4 ++-- >> fs/xfs/xfs_ioctl.c | 4 ++-- >> 4 files changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h >> index 2a2e3cfd94f0..a17313efc1fe 100644 >> --- a/fs/xfs/libxfs/xfs_fs.h >> +++ b/fs/xfs/libxfs/xfs_fs.h >> @@ -308,12 +308,12 @@ struct xfs_ag_geometry { >> typedef struct xfs_growfs_data { >> __u64 newblocks; /* new data subvol size, fsblocks */ >> __u32 imaxpct; /* new inode space percentage limit */ >> -} xfs_growfs_data_t; >> +}; > > So long as Eric is ok with fixing this up in xfs_fs_compat.h in > userspace, > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> Sure, why not :) (tho is growfs really a public interface? I guess so, technically, though not documented as such.) Reviewed-by: Eric Sandeen <sandeen@redhat.com> -Eric
On Fri, Jan 08, 2021 at 03:27:21PM -0600, Eric Sandeen wrote: > On 1/8/21 3:21 PM, Darrick J. Wong wrote: > > On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote: > >> Such usage isn't encouraged by the kernel coding style. > >> > >> Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > >> --- > >> fs/xfs/libxfs/xfs_fs.h | 4 ++-- > >> fs/xfs/xfs_fsops.c | 12 ++++++------ > >> fs/xfs/xfs_fsops.h | 4 ++-- > >> fs/xfs/xfs_ioctl.c | 4 ++-- > >> 4 files changed, 12 insertions(+), 12 deletions(-) > >> > >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > >> index 2a2e3cfd94f0..a17313efc1fe 100644 > >> --- a/fs/xfs/libxfs/xfs_fs.h > >> +++ b/fs/xfs/libxfs/xfs_fs.h > >> @@ -308,12 +308,12 @@ struct xfs_ag_geometry { > >> typedef struct xfs_growfs_data { > >> __u64 newblocks; /* new data subvol size, fsblocks */ > >> __u32 imaxpct; /* new inode space percentage limit */ > >> -} xfs_growfs_data_t; > >> +}; > > > > So long as Eric is ok with fixing this up in xfs_fs_compat.h in > > userspace, > > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > Sure, why not :) (tho is growfs really a public interface? I guess so, > technically, though not documented as such.) Yeah, although I think nobody else uses it (I could leave the typedef definitions only if needed otherwise...) Thanks, Gao Xiang > > Reviewed-by: Eric Sandeen <sandeen@redhat.com> > > -Eric >
On Sat, Jan 09, 2021 at 08:49:34AM +0800, Gao Xiang wrote: > On Fri, Jan 08, 2021 at 03:27:21PM -0600, Eric Sandeen wrote: > > On 1/8/21 3:21 PM, Darrick J. Wong wrote: > > > On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote: > > >> Such usage isn't encouraged by the kernel coding style. > > >> > > >> Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > >> --- > > >> fs/xfs/libxfs/xfs_fs.h | 4 ++-- > > >> fs/xfs/xfs_fsops.c | 12 ++++++------ > > >> fs/xfs/xfs_fsops.h | 4 ++-- > > >> fs/xfs/xfs_ioctl.c | 4 ++-- > > >> 4 files changed, 12 insertions(+), 12 deletions(-) > > >> > > >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > > >> index 2a2e3cfd94f0..a17313efc1fe 100644 > > >> --- a/fs/xfs/libxfs/xfs_fs.h > > >> +++ b/fs/xfs/libxfs/xfs_fs.h > > >> @@ -308,12 +308,12 @@ struct xfs_ag_geometry { > > >> typedef struct xfs_growfs_data { > > >> __u64 newblocks; /* new data subvol size, fsblocks */ > > >> __u32 imaxpct; /* new inode space percentage limit */ > > >> -} xfs_growfs_data_t; > > >> +}; > > > > > > So long as Eric is ok with fixing this up in xfs_fs_compat.h in > > > userspace, > > > > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > Sure, why not :) (tho is growfs really a public interface? I guess so, > > technically, though not documented as such.) They are not described in man pages, though they are listed in xfsctl(3) so they are definitely public interfaces. > Yeah, although I think nobody else uses it (I could leave the typedef > definitions only if needed otherwise...) It is used elsewhere - ISTR that it is used by a couple of third party applications that integrate growing filesystems into their other storage management tasks. Cheers, Dave.
On Mon, Jan 11, 2021 at 08:04:36AM +1100, Dave Chinner wrote: > On Sat, Jan 09, 2021 at 08:49:34AM +0800, Gao Xiang wrote: > > On Fri, Jan 08, 2021 at 03:27:21PM -0600, Eric Sandeen wrote: > > > On 1/8/21 3:21 PM, Darrick J. Wong wrote: > > > > On Sat, Jan 09, 2021 at 03:09:17AM +0800, Gao Xiang wrote: > > > >> Such usage isn't encouraged by the kernel coding style. > > > >> > > > >> Signed-off-by: Gao Xiang <hsiangkao@redhat.com> > > > >> --- > > > >> fs/xfs/libxfs/xfs_fs.h | 4 ++-- > > > >> fs/xfs/xfs_fsops.c | 12 ++++++------ > > > >> fs/xfs/xfs_fsops.h | 4 ++-- > > > >> fs/xfs/xfs_ioctl.c | 4 ++-- > > > >> 4 files changed, 12 insertions(+), 12 deletions(-) > > > >> > > > >> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h > > > >> index 2a2e3cfd94f0..a17313efc1fe 100644 > > > >> --- a/fs/xfs/libxfs/xfs_fs.h > > > >> +++ b/fs/xfs/libxfs/xfs_fs.h > > > >> @@ -308,12 +308,12 @@ struct xfs_ag_geometry { > > > >> typedef struct xfs_growfs_data { > > > >> __u64 newblocks; /* new data subvol size, fsblocks */ > > > >> __u32 imaxpct; /* new inode space percentage limit */ > > > >> -} xfs_growfs_data_t; > > > >> +}; > > > > > > > > So long as Eric is ok with fixing this up in xfs_fs_compat.h in > > > > userspace, > > > > > > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > Sure, why not :) (tho is growfs really a public interface? I guess so, > > > technically, though not documented as such.) > > They are not described in man pages, though they are listed in > xfsctl(3) so they are definitely public interfaces. > > > Yeah, although I think nobody else uses it (I could leave the typedef > > definitions only if needed otherwise...) > > It is used elsewhere - ISTR that it is used by a couple of third > party applications that integrate growing filesystems into their > other storage management tasks. Okay, will leave the definitions in the next version. Thanks, Gao Xiang > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com >
On Mon, Jan 11, 2021 at 08:17:49AM +0800, Gao Xiang wrote:
> Okay, will leave the definitions in the next version.
Note that the important thing is to not break the kernel to userspace
ABI, the API is a little less important. That being said breaking
programs piecemail for no good reason't isn't nice. I think we
should probably move all the typedefs to the end of the file with
a comment and then at some point remove them as a single breaking
change.
On Mon, Jan 11, 2021 at 05:30:54PM +0000, Christoph Hellwig wrote: > On Mon, Jan 11, 2021 at 08:17:49AM +0800, Gao Xiang wrote: > > Okay, will leave the definitions in the next version. > > Note that the important thing is to not break the kernel to userspace > ABI, the API is a little less important. That being said breaking > programs piecemail for no good reason't isn't nice. I think we > should probably move all the typedefs to the end of the file with > a comment and then at some point remove them as a single breaking > change. Agree with your point. Yet I'm not sure if it's much related to this series (I raised this due to touch some code)... I think it'd be better as an independent xfs-treewide patch to move all userspace visable typedefs to a unique place at once (not just these two typedefs...) But I'm also ok if just move these two types in advance if needed. (I suffer from lack of modification of old typedefs out of copy & paste from time to time. IMHO, I hope such exist typedef usage could be removed as soon as possible... ) Thanks, Gao Xiang >
diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h index 2a2e3cfd94f0..a17313efc1fe 100644 --- a/fs/xfs/libxfs/xfs_fs.h +++ b/fs/xfs/libxfs/xfs_fs.h @@ -308,12 +308,12 @@ struct xfs_ag_geometry { typedef struct xfs_growfs_data { __u64 newblocks; /* new data subvol size, fsblocks */ __u32 imaxpct; /* new inode space percentage limit */ -} xfs_growfs_data_t; +}; typedef struct xfs_growfs_log { __u32 newblocks; /* new log size, fsblocks */ __u32 isint; /* 1 if new log is internal */ -} xfs_growfs_log_t; +}; typedef struct xfs_growfs_rt { __u64 newblocks; /* new realtime size, fsblocks */ diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c index d254588f6e21..6c5f6a50da2e 100644 --- a/fs/xfs/xfs_fsops.c +++ b/fs/xfs/xfs_fsops.c @@ -25,8 +25,8 @@ */ static int xfs_growfs_data_private( - xfs_mount_t *mp, /* mount point for filesystem */ - xfs_growfs_data_t *in) /* growfs data input struct */ + struct xfs_mount *mp, /* mount point for filesystem */ + struct xfs_growfs_data *in) /* growfs data input struct */ { struct xfs_buf *bp; int error; @@ -35,7 +35,7 @@ xfs_growfs_data_private( xfs_rfsblock_t nb, nb_mod; xfs_rfsblock_t delta; xfs_agnumber_t oagcount; - xfs_trans_t *tp; + struct xfs_trans *tp; struct aghdr_init_data id = {}; nb = in->newblocks; @@ -170,8 +170,8 @@ xfs_growfs_data_private( static int xfs_growfs_log_private( - xfs_mount_t *mp, /* mount point for filesystem */ - xfs_growfs_log_t *in) /* growfs log input struct */ + struct xfs_mount *mp, /* mount point for filesystem */ + struct xfs_growfs_log *in) /* growfs log input struct */ { xfs_extlen_t nb; @@ -268,7 +268,7 @@ xfs_growfs_data( int xfs_growfs_log( xfs_mount_t *mp, - xfs_growfs_log_t *in) + struct xfs_growfs_log *in) { int error; diff --git a/fs/xfs/xfs_fsops.h b/fs/xfs/xfs_fsops.h index 92869f6ec8d3..d7e9af4a28eb 100644 --- a/fs/xfs/xfs_fsops.h +++ b/fs/xfs/xfs_fsops.h @@ -6,8 +6,8 @@ #ifndef __XFS_FSOPS_H__ #define __XFS_FSOPS_H__ -extern int xfs_growfs_data(xfs_mount_t *mp, xfs_growfs_data_t *in); -extern int xfs_growfs_log(xfs_mount_t *mp, xfs_growfs_log_t *in); +extern int xfs_growfs_data(xfs_mount_t *mp, struct xfs_growfs_data *in); +extern int xfs_growfs_log(xfs_mount_t *mp, struct xfs_growfs_log *in); extern void xfs_fs_counts(xfs_mount_t *mp, xfs_fsop_counts_t *cnt); extern int xfs_reserve_blocks(xfs_mount_t *mp, uint64_t *inval, xfs_fsop_resblks_t *outval); diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 3fbd98f61ea5..a62520f49ec5 100644 --- a/fs/xfs/xfs_ioctl.c +++ b/fs/xfs/xfs_ioctl.c @@ -2260,7 +2260,7 @@ xfs_file_ioctl( } case XFS_IOC_FSGROWFSDATA: { - xfs_growfs_data_t in; + struct xfs_growfs_data in; if (copy_from_user(&in, arg, sizeof(in))) return -EFAULT; @@ -2274,7 +2274,7 @@ xfs_file_ioctl( } case XFS_IOC_FSGROWFSLOG: { - xfs_growfs_log_t in; + struct xfs_growfs_log in; if (copy_from_user(&in, arg, sizeof(in))) return -EFAULT;
Such usage isn't encouraged by the kernel coding style. Signed-off-by: Gao Xiang <hsiangkao@redhat.com> --- fs/xfs/libxfs/xfs_fs.h | 4 ++-- fs/xfs/xfs_fsops.c | 12 ++++++------ fs/xfs/xfs_fsops.h | 4 ++-- fs/xfs/xfs_ioctl.c | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-)