xfs
[Top] [All Lists]

Re: [PATCH v5 06/10] xfs: add XFS_IOC_FREE_EOFBLOCKS ioctl

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH v5 06/10] xfs: add XFS_IOC_FREE_EOFBLOCKS ioctl
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Wed, 24 Oct 2012 12:16:55 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121023013104.GJ4291@dastard>
References: <1349446636-8611-1-git-send-email-bfoster@xxxxxxxxxx> <1349446636-8611-7-git-send-email-bfoster@xxxxxxxxxx> <20121023013104.GJ4291@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1
On 10/22/2012 09:31 PM, Dave Chinner wrote:
> On Fri, Oct 05, 2012 at 10:17:12AM -0400, Brian Foster wrote:
...
>>  /*
>> + * Speculative preallocation trimming.
>> + */
>> +#define XFS_EOFBLOCKS_VERSION               1
>> +struct xfs_eofblocks {
>> +    __u32           eof_version;
>> +    __u32           eof_flags;
>> +    unsigned char   pad[12];
>> +};
> 
> 12 bytes of padding is a bit wierd at this point. It's 
> problematic for 32bit userspace on 64 bit kernels, in that the size
> of the structure can end up different (i.e. 20 bytes on 32b, 24 bytes
> on 64b) depending on the architectures natural alignment.
> 
> I can also see that adding multiple extra variables to the structure
> are quite likely (e.g. per-ag control, start/end inode numbers,
> etc), so 12 bytes of padding really isn't sufficient, IMO. I'd tend
> to pad out to, say, 128 bytes rather than 32, just in case. i.e:
> 
>       __u64           pad[15];
> 
> And then take away from this padding space as you add functioanlity
> in fucture patches.
> 
> This extra padding means the version number won't need to increase
> any time soon, as it will be a while before we run out of either
> padding or flag space, instead of as soon as we add a new function
> to the ioctl....
> 

Ok, I'll convert the file size parameter to a __u64 (as per discussed in
the spaceman eofblocks command thread) and expand the padding.

>> +
>> +/* eof_flags values */
>> +#define XFS_EOF_FLAGS_SYNC          0x01    /* sync/wait mode scan */
> 
> I kind of prefer flags being defined by (1 << 0) style to keep
> larger flag numbers concise, but that's not a big deal.
> 

Eh, I find this cleaner than looking at the raw values as well.

>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 0e0232c..ad4352f 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -42,6 +42,7 @@
>>  #include "xfs_inode_item.h"
>>  #include "xfs_export.h"
>>  #include "xfs_trace.h"
>> +#include "xfs_icache.h"
>>  
>>  #include <linux/capability.h>
>>  #include <linux/dcache.h>
>> @@ -1602,6 +1603,21 @@ xfs_file_ioctl(
>>              error = xfs_errortag_clearall(mp, 1);
>>              return -error;
>>  
>> +    case XFS_IOC_FREE_EOFBLOCKS: {
>> +            struct xfs_eofblocks eofb;
>> +            int flags;
>> +
>> +            if (copy_from_user(&eofb, arg, sizeof(eofb)))
>> +                    return -XFS_ERROR(EFAULT);
>> +
>> +            if (eofb.eof_version != XFS_EOFBLOCKS_VERSION)
>> +                    return -XFS_ERROR(EINVAL);
> 
> Checking that no unsupported flags are set here is necessary. Also,
> checking the padding is zero is probably a good idea, as it will
> force applications to zero the structure properly before being able
> to use this interface properly....
> 

Ok.

>> +            flags = (eofb.eof_flags & XFS_EOF_FLAGS_SYNC) ? SYNC_WAIT : 
>> SYNC_TRYLOCK;
> 
> Line if a bit too long. However, would it be better to place this
> inside xfs_icache_free_eofblocks()?
> 

Are you suggesting to eliminate the flags parameter to
xfs_icache_free_eofblocks? The reason for the current interface is that
the background scan caller doesn't require the eofb parameter, so I
decided to generalize the sync/wait parameter in the caller.

If we want to push it into xfs_icache_free_eofblocks(), I think it would
be better at that point to eliminate the flags param and either infer
SYNC_TRYLOCK on a NULL eofb or to require an eofb and pass one with
a cleared XFS_EOF_FLAGS_SYNC from the background scan. Thoughts?

Brian

>> +            error = xfs_icache_free_eofblocks(mp, flags, &eofb);
> 
> Cheers,
> 
> Dave.
> 

<Prev in Thread] Current Thread [Next in Thread>