On Thu, Nov 30, 2017 at 11:02:51AM +1100, Dave Chinner wrote:
> On Wed, Nov 29, 2017 at 03:48:50PM -0700, Allison Henderson wrote:
> >
> >
> > On 11/29/2017 02:37 PM, Dave Chinner wrote:
> > >On Tue, Nov 28, 2017 at 12:35:37PM -0800, Darrick J. Wong wrote:
> > >>On Fri, Nov 17, 2017 at 11:21:45AM -0700, Allison Henderson wrote:
> > >>>This patch adds a new file ioctl to retrieve the parent
> > >>>pointer of a given inode
> > >>
> > >>(Yes, it's time to start talking about actual use cases...)
> > >>
> > >>At a bare minimum, this is what I pictured for the "return parents of
> > >>the open file" ioctl:
> > >>
> > >>#define XFS_PPTR_MAXNAMELEN 255
> > >>
> > >>struct xfs_pptr {
> > >> u64 pp_ino;
> > >> u32 pp_gen;
> > >> u8 pp_namelen;
> > >> u8 pp_name[XFS_PPTR_MAXNAMELEN];
> > >>};
> > >
> > >That's going to be a different size on 32bit and 64 bit platforms
> > >as the structure size is a multiple of 4 bytes, not 8 bytes.
> > >That will cause problems and need complex comapt ioctl translation.
> > >Better to make pp_namelen a u32 and that will make the structure
> > >64 bit aligned and sized on all platforms.
> > >
> > >I'd allow more than u8 for the namelen. Yes, while we currently
> > >allow on 255 bytes for a name, it would make more sense to
> > >use a u32 here so that the structure size is a multiple of it's
> > >alignment rather than having a 4 byte hole in the array we don't
> > >fill out....
Maybe this ought to get padded up to the nearest 8-byte boundary too.
> > >
> > >>
> > >>/* return parents of the handle, instead of the open fd */
> > >>#define XFS_PPTR_FLAG_HANDLE (1u << 0)
> > >>
> > >>struct xfs_pptr_info {
> > >> struct xfs_fsop_handlereq pi_handle;
> > >> struct xfs_attrlist_cursor pi_cursor;
> > >> u32 pi_flags;
> > >> u32 pi_reserved;
> > >> u32 pi_ptrs_size;
> > >> u32 pi_ptrs_used;
> > >> u64 pi_reserved2[6];
> > >> struct xfs_pptr pi_ptrs[0];
> > >>};
> > >
> > >I thought gcc had started doing weird things with variable size
> > >array declarations like this (i.e. pi_ptrs[0]) because the exact
> > >behaviour is not defined in the C standard. i.e. we need to avoid
> > >adding new declarations that do this...
> >
> > Oh, I think there's a few places in the set where I have
> > declarations like that.
>
> Yup, there are quite a few, but IIRC we can't rely on them working
> as they do right now in future compilers. So I'm pretty sure we need
> to avoid these sorts of constructs if we can. Doing something like
> this:
If gcc starts bungling them, there's going to be a lot of stuff in
include/uapi/ that breaks. FIEMAP, FSMAP, the weird vfs dedupe ioctl...
I think it'll be fine so long as we keep an eye on the structure size
in xfs_ondisk.h. If the structure size mutates we'll know because the
ioctl will stop working with old userspace and/or we fail the build.
Oh but we don't keep an eye on that stuff. Sigh.
> struct xfs_pptr_info {
> struct xfs_fsop_handlereq pi_handle;
> struct xfs_attrlist_cursor pi_cursor;
> u32 pi_flags;
> u32 pi_reserved;
> u32 pi_ptrs_size;
> u32 pi_ptrs_used;
> u64 pi_reserved2[6];
>
> /*
> * An array of struct xfs_pptr follows the header
> * information. Use XFS_PPINFO_TO_PP() to access the
> * parent pointer array entries.
> */
> };
>
> And providing an accessor function:
>
> #define XFS_PPINFO_TO_PP(info, idx) \
> (&(((struct xfs_pptr *)((char *)(info) + sizeof(*(info))))[(idx)]))
Eww, macros. :)
static inline struct xfs_pptr *
xfs_ppinfo_to_pp(
struct xfs_pptr_info *info,
unsigned int idx)
{
return (struct xfs_pptr *)((char *)(info + 1)) + (idx * sizeof(struct xfs_pptr));
}
> Will solve the problem.
>
> > Should they be some_array[1]; instead?
>
> That has problems, too. See, for example, commit ffeecc521302 ("xfs:
> Fix xfs_attr_leafblock definition"), where gcc completely mangled
> the code because it thought it could optimise away bits of the
> structure and code that "weren't used".
Especially no on the some_array[1], that bit us with the v5 AGFL...
> > >>#define XFS_PPTR_INFO_SIZEOF(ptrs) (sizeof(struct xfs_pptr_info) + \
> > >> ((ptrs) * sizeof(struct xfs_pptr)));
> > >>static inline struct xfs_pptr_info *
> > >>xfs_pptr_alloc(
> > >> size_t nr_ptrs)
> > >>{
> > >> struct xfs_pptr_info *ppi;
> > >>
> > >> ppi = malloc(XFS_PPTR_INFO_SIZEOF(nr_ptrs));
> > >> if (!ppi)
> > >> return NULL;
> > >> memset(ppi, 0, XFS_PPTR_INFO_SIZEOF(nr_ptrs));
> > >> ppi->pi_ptrs_size = nr_ptrs;
> > >> return ppi;
> > >>}
> > >>
> > >>With the following example userspace program (that does no checking
> > >>whatsoever):
> > >>
> > >>int main(int argc, char *argv[])
> > >>{
> > >> struct xfs_pptr_info *ppi;
> > >> struct xfs_pptr *pp;
> > >> int fd;
> > >>
> > >> fd = open(argv[1], O_RDONLY);
> > >> ppi = xfs_pptr_alloc(32);
> > >>
> > >> while (ioctl(fd, XFS_IOC_GETPPOINTER, ppi) == 0 && ppi->pi_ptrs_used) {
> > >> for (i = 0; i < ppi->pi_ptrs_used; i++) {
> > >> printf("%llu:%u -> %s\n",
> > >> ppi->pi_ptrs[i].pp_ino,
> > >> ppi->pi_ptrs[i].pp_gen,
> > >> ppi->pi_ptrs[i].pp_name);
>
> And this becomes:
>
> for (i = 0; i < ppi->pi_ptrs_used; i++) {
> pp = XFS_PPINFO_TO_PP(ppi, i);
> printf("%llu:%u -> %s\n", pp->pp_ino, pp->pp_gen,
> pp->pp_name);
> }
Funnily enough I've added more bits to this, maybe I should just send a
real RFC patch to the list.
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
