[PATCH] xfsprogs: fix geometry calls on older kernels for 5.2.1

[PATCH] xfsprogs: fix geometry calls on older kernels for 5.2.1

[Eric Sandeen]

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?

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 */

Re: [PATCH] xfsprogs: fix geometry calls on older kernels for 5.2.1

[Darrick J. Wong]

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 */
> 

Re: [PATCH] xfsprogs: fix geometry calls on older kernels for 5.2.1

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfsprogs: fix geometry calls on older kernels for 5.2.1

[Eric Sandeen]

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