| Message ID | 7d83cd0d-8a15-201e-9ebf-e1f859270b92@sandeen.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfsprogs: fix geometry calls on older kernels for 5.2.1 | expand |
On Tue, Aug 20, 2019 at 03:47:29PM -0500, Eric Sandeen wrote: > I didn't think 5.2.0 through; the udpate of the geometry ioctl means > that the tools won't work on older kernels that don't support the > v5 ioctls, since I failed to merge Darrick's wrappers. > > As a very quick one-off I'd like to merge this to just revert every > geometry call back to the original ioctl, so it keeps working on > older kernels and I'll release 5.2.1. This hack can go away when > Darrick's wrappers get merged. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> For the four line code fix, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > > I'm a little concerned that 3rd party existing code which worked fine > before will now get the new XFS_IOC_FSGEOMETRY definition if they get > rebuilt, and suddenly stop working on older kernels. Am I overreacting > or misunderstanding our compatibility goals? As for this question ^^^ ... <URRRK>. I thought the overall strategy was to get everything in xfsprogs using libfrog wrappers that would degrade gracefully on old kernels. For xfsdump/restore, I think we should just merge it into xfsprogs and then it can use our wrappers. For everything else... I thought the story was that you shouldn't really be using xfs ioctls unless you're keeping up with upstream. <shrug> Feel free to differ, that's just a braindump of my shattered mind. :P --D > diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h > index f1158a79..253b706c 100644 > --- a/libxfs/xfs_fs.h > +++ b/libxfs/xfs_fs.h > @@ -720,7 +720,10 @@ struct xfs_scrub_metadata { > #define XFS_IOC_ATTRMULTI_BY_HANDLE _IOW ('X', 123, struct xfs_fsop_attrmulti_handlereq) > #define XFS_IOC_FSGEOMETRY_V4 _IOR ('X', 124, struct xfs_fsop_geom_v4) > #define XFS_IOC_GOINGDOWN _IOR ('X', 125, uint32_t) > -#define XFS_IOC_FSGEOMETRY _IOR ('X', 126, struct xfs_fsop_geom) > +/* For backwards compatibility in 5.2.1, just for now */ > +/* #define XFS_IOC_FSGEOMETRY _IOR ('X', 126, struct xfs_fsop_geom_v5) */ > +#define XFS_IOC_FSGEOMETRY XFS_IOC_FSGEOMETRY_V4 > + > /* XFS_IOC_GETFSUUID ---------- deprecated 140 */ > > /* reflink ioctls; these MUST match the btrfs ioctl definitions */ >
On Tue, Aug 20, 2019 at 02:18:28PM -0700, Darrick J. Wong wrote: > On Tue, Aug 20, 2019 at 03:47:29PM -0500, Eric Sandeen wrote: > > I didn't think 5.2.0 through; the udpate of the geometry ioctl means > > that the tools won't work on older kernels that don't support the > > v5 ioctls, since I failed to merge Darrick's wrappers. > > > > As a very quick one-off I'd like to merge this to just revert every > > geometry call back to the original ioctl, so it keeps working on > > older kernels and I'll release 5.2.1. This hack can go away when > > Darrick's wrappers get merged. > > > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > > For the four line code fix, > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > > I'm a little concerned that 3rd party existing code which worked fine > > before will now get the new XFS_IOC_FSGEOMETRY definition if they get > > rebuilt, and suddenly stop working on older kernels. Am I overreacting > > or misunderstanding our compatibility goals? > > As for this question ^^^ ... <URRRK>. > > I thought the overall strategy was to get everything in xfsprogs using > libfrog wrappers that would degrade gracefully on old kernels. The wrappers were a necessary part of the conversion. They should have been merged with the rest of XFS_IOC_FSGEOMETRY changes. How did this get broken up? > For xfsdump/restore, I think we should just merge it into xfsprogs and > then it can use our wrappers. Don't need to care about dump/restore: $ git grep FSGEOM common/fs.c: if (ioctl(fd, XFS_IOC_FSGEOMETRY_V1, &geo)) { doc/CHANGES: XFS_IOC_FSGEOMETRY instead of XFS_IOC_GETFSUUID ioctl, so $ It only uses teh V1 ioctl. As it is, the correct thing to do here is to put the fallback into the xfsctl() function. This is actually an exported and documented interface to use xfs ioctls by external problems - it's part of libhandle(), and that should be obvious by the fact the man page that describes all this is xfsctl(3). i.e. any app using XFS ioctls should be using the xfsctl() interface, not calling ioctl directly. The whole reason for that it because it allows us to handle things like this in application independent code.... So I'd suggest that the fallback code should be in the xfsctl handler and then userspace will pick this up and won't care about which kernel it is running on... I suspect the bigger picture is to convert all the open ioctl() calls in xfsprogs for XFS specific ioctls to xfsctl(). We've kinda screwed this pooch since we stopped having to support multiple platforms. > For everything else... I thought the story was that you shouldn't really > be using xfs ioctls unless you're keeping up with upstream. Or you should be linked against libhandle and using xfsctl() to be isolated from these sorts of things. Cheers, Dave.
On 8/20/19 5:46 PM, Dave Chinner wrote: > On Tue, Aug 20, 2019 at 02:18:28PM -0700, Darrick J. Wong wrote: >> On Tue, Aug 20, 2019 at 03:47:29PM -0500, Eric Sandeen wrote: >>> I didn't think 5.2.0 through; the udpate of the geometry ioctl means >>> that the tools won't work on older kernels that don't support the >>> v5 ioctls, since I failed to merge Darrick's wrappers. >>> >>> As a very quick one-off I'd like to merge this to just revert every >>> geometry call back to the original ioctl, so it keeps working on >>> older kernels and I'll release 5.2.1. This hack can go away when >>> Darrick's wrappers get merged. >>> >>> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> >> For the four line code fix, >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> >> >>> --- >>> >>> I'm a little concerned that 3rd party existing code which worked fine >>> before will now get the new XFS_IOC_FSGEOMETRY definition if they get >>> rebuilt, and suddenly stop working on older kernels. Am I overreacting >>> or misunderstanding our compatibility goals? >> >> As for this question ^^^ ... <URRRK>. >> >> I thought the overall strategy was to get everything in xfsprogs using >> libfrog wrappers that would degrade gracefully on old kernels. > > The wrappers were a necessary part of the conversion. They should > have been merged with the rest of XFS_IOC_FSGEOMETRY changes. How > did this get broken up? because libxfs sync is automated, and wrappers are not. Darrick tried, but despite his best efforts my suckiness prevailed spectacularly this time. Anyway my worry was about 3rd parties directly using the ioctl definition but I guess I've been convinced that I don't need to worry about that because it's not for 3rd party consumption in general. >> For xfsdump/restore, I think we should just merge it into xfsprogs and >> then it can use our wrappers. > > Don't need to care about dump/restore: > > $ git grep FSGEOM > common/fs.c: if (ioctl(fd, XFS_IOC_FSGEOMETRY_V1, &geo)) { > doc/CHANGES: XFS_IOC_FSGEOMETRY instead of XFS_IOC_GETFSUUID ioctl, so > $ > > It only uses teh V1 ioctl. Yup, thanks for checking that. > As it is, the correct thing to do here is to put the fallback into > the xfsctl() function. This is actually an exported and documented > interface to use xfs ioctls by external problems - it's part of > libhandle(), and that should be obvious by the fact the man page > that describes all this is xfsctl(3). > > i.e. any app using XFS ioctls should be using the xfsctl() > interface, not calling ioctl directly. The whole reason for that it > because it allows us to handle things like this in application > independent code.... > > So I'd suggest that the fallback code should be in the xfsctl > handler and then userspace will pick this up and won't care about > which kernel it is running on... > > I suspect the bigger picture is to convert all the open ioctl() > calls in xfsprogs for XFS specific ioctls to xfsctl(). We've kinda > screwed this pooch since we stopped having to support multiple > platforms. > >> For everything else... I thought the story was that you shouldn't really >> be using xfs ioctls unless you're keeping up with upstream. > > Or you should be linked against libhandle and using xfsctl() to > be isolated from these sorts of things. Yeah fair. Thanks, -Eric
diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h index f1158a79..253b706c 100644 --- a/libxfs/xfs_fs.h +++ b/libxfs/xfs_fs.h @@ -720,7 +720,10 @@ struct xfs_scrub_metadata { #define XFS_IOC_ATTRMULTI_BY_HANDLE _IOW ('X', 123, struct xfs_fsop_attrmulti_handlereq) #define XFS_IOC_FSGEOMETRY_V4 _IOR ('X', 124, struct xfs_fsop_geom_v4) #define XFS_IOC_GOINGDOWN _IOR ('X', 125, uint32_t) -#define XFS_IOC_FSGEOMETRY _IOR ('X', 126, struct xfs_fsop_geom) +/* For backwards compatibility in 5.2.1, just for now */ +/* #define XFS_IOC_FSGEOMETRY _IOR ('X', 126, struct xfs_fsop_geom_v5) */ +#define XFS_IOC_FSGEOMETRY XFS_IOC_FSGEOMETRY_V4 + /* XFS_IOC_GETFSUUID ---------- deprecated 140 */ /* reflink ioctls; these MUST match the btrfs ioctl definitions */
I didn't think 5.2.0 through; the udpate of the geometry ioctl means that the tools won't work on older kernels that don't support the v5 ioctls, since I failed to merge Darrick's wrappers. As a very quick one-off I'd like to merge this to just revert every geometry call back to the original ioctl, so it keeps working on older kernels and I'll release 5.2.1. This hack can go away when Darrick's wrappers get merged. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- I'm a little concerned that 3rd party existing code which worked fine before will now get the new XFS_IOC_FSGEOMETRY definition if they get rebuilt, and suddenly stop working on older kernels. Am I overreacting or misunderstanding our compatibility goals?