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@xxxxxxxxxx>
>>
>> For the four line code fix,
>> Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>>
>>> ---
>>>
>>> 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
