| Message ID | 20191217023535.GA12765@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxfs: make resync with the userspace libxfs easier | expand |
On 12/16/19 8:35 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Prepare to resync the userspace libxfs with the kernel libxfs. There > were a few things I missed -- a couple of static inline directory > functions that have to be exported for xfs_repair; a couple of directory > naming functions that make porting much easier if they're /not/ static > inline; and a u16 usage that should have been uint16_t. > > None of these things are bugs in their own right; this just makes > porting xfsprogs easier. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> This all seems fine - I'm wondering about prototypes in xfs_dir2_priv.h vs xfs_dir2.h - at this point I'm not sure why we have the two, help? But whatevs, if that needs cleanup it can come later. Reviewed-by: Eric Sandeen <sandeen@redhat.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 2 +- > fs/xfs/libxfs/xfs_dir2.c | 21 +++++++++++++++++++++ > fs/xfs/libxfs/xfs_dir2_priv.h | 29 +++++++++-------------------- > fs/xfs/libxfs/xfs_dir2_sf.c | 6 +++--- > 4 files changed, 34 insertions(+), 24 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index 4a802b3abe77..4c2e046fbfad 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -4561,7 +4561,7 @@ xfs_bmapi_convert_delalloc( > struct xfs_mount *mp = ip->i_mount; > xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset); > struct xfs_bmalloca bma = { NULL }; > - u16 flags = 0; > + uint16_t flags = 0; > struct xfs_trans *tp; > int error; > > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c > index 0aa87cbde49e..dd6fcaaea318 100644 > --- a/fs/xfs/libxfs/xfs_dir2.c > +++ b/fs/xfs/libxfs/xfs_dir2.c > @@ -724,3 +724,24 @@ xfs_dir2_namecheck( > /* There shouldn't be any slashes or nulls here */ > return !memchr(name, '/', length) && !memchr(name, 0, length); > } > + > +xfs_dahash_t > +xfs_dir2_hashname( > + struct xfs_mount *mp, > + struct xfs_name *name) > +{ > + if (unlikely(xfs_sb_version_hasasciici(&mp->m_sb))) > + return xfs_ascii_ci_hashname(name); > + return xfs_da_hashname(name->name, name->len); > +} > + > +enum xfs_dacmp > +xfs_dir2_compname( > + struct xfs_da_args *args, > + const unsigned char *name, > + int len) > +{ > + if (unlikely(xfs_sb_version_hasasciici(&args->dp->i_mount->m_sb))) > + return xfs_ascii_ci_compname(args, name, len); > + return xfs_da_compname(args, name, len); > +} > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h > index c031c53d0f0d..01ee0b926572 100644 > --- a/fs/xfs/libxfs/xfs_dir2_priv.h > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h > @@ -175,6 +175,12 @@ extern int xfs_dir2_sf_lookup(struct xfs_da_args *args); > extern int xfs_dir2_sf_removename(struct xfs_da_args *args); > extern int xfs_dir2_sf_replace(struct xfs_da_args *args); > extern xfs_failaddr_t xfs_dir2_sf_verify(struct xfs_inode *ip); > +int xfs_dir2_sf_entsize(struct xfs_mount *mp, > + struct xfs_dir2_sf_hdr *hdr, int len); > +void xfs_dir2_sf_put_ino(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *hdr, > + struct xfs_dir2_sf_entry *sfep, xfs_ino_t ino); > +void xfs_dir2_sf_put_ftype(struct xfs_mount *mp, > + struct xfs_dir2_sf_entry *sfep, uint8_t ftype); > > /* xfs_dir2_readdir.c */ > extern int xfs_readdir(struct xfs_trans *tp, struct xfs_inode *dp, > @@ -194,25 +200,8 @@ xfs_dir2_data_entsize( > return round_up(len, XFS_DIR2_DATA_ALIGN); > } > > -static inline xfs_dahash_t > -xfs_dir2_hashname( > - struct xfs_mount *mp, > - struct xfs_name *name) > -{ > - if (unlikely(xfs_sb_version_hasasciici(&mp->m_sb))) > - return xfs_ascii_ci_hashname(name); > - return xfs_da_hashname(name->name, name->len); > -} > - > -static inline enum xfs_dacmp > -xfs_dir2_compname( > - struct xfs_da_args *args, > - const unsigned char *name, > - int len) > -{ > - if (unlikely(xfs_sb_version_hasasciici(&args->dp->i_mount->m_sb))) > - return xfs_ascii_ci_compname(args, name, len); > - return xfs_da_compname(args, name, len); > -} > +xfs_dahash_t xfs_dir2_hashname(struct xfs_mount *mp, struct xfs_name *name); > +enum xfs_dacmp xfs_dir2_compname(struct xfs_da_args *args, > + const unsigned char *name, int len); > > #endif /* __XFS_DIR2_PRIV_H__ */ > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c > index 8b94d33d232f..7b7f6fb2ea3b 100644 > --- a/fs/xfs/libxfs/xfs_dir2_sf.c > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c > @@ -37,7 +37,7 @@ static void xfs_dir2_sf_check(xfs_da_args_t *args); > static void xfs_dir2_sf_toino4(xfs_da_args_t *args); > static void xfs_dir2_sf_toino8(xfs_da_args_t *args); > > -static int > +int > xfs_dir2_sf_entsize( > struct xfs_mount *mp, > struct xfs_dir2_sf_hdr *hdr, > @@ -84,7 +84,7 @@ xfs_dir2_sf_get_ino( > return get_unaligned_be64(from) & XFS_MAXINUMBER; > } > > -static void > +void > xfs_dir2_sf_put_ino( > struct xfs_mount *mp, > struct xfs_dir2_sf_hdr *hdr, > @@ -145,7 +145,7 @@ xfs_dir2_sf_get_ftype( > return XFS_DIR3_FT_UNKNOWN; > } > > -static void > +void > xfs_dir2_sf_put_ftype( > struct xfs_mount *mp, > struct xfs_dir2_sf_entry *sfep, >
On Wed, Dec 18, 2019 at 03:00:14PM -0600, Eric Sandeen wrote: > On 12/16/19 8:35 PM, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Prepare to resync the userspace libxfs with the kernel libxfs. There > > were a few things I missed -- a couple of static inline directory > > functions that have to be exported for xfs_repair; a couple of directory > > naming functions that make porting much easier if they're /not/ static > > inline; and a u16 usage that should have been uint16_t. > > > > None of these things are bugs in their own right; this just makes > > porting xfsprogs easier. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > This all seems fine - I'm wondering about prototypes in xfs_dir2_priv.h > vs xfs_dir2.h - at this point I'm not sure why we have the two, > help? I think the xfs_dir2_priv header is for symbols that are internal to directories, whereas xfs_dir2.h is for functions that higher level functions should call to create/modify/erase directory entries. > But whatevs, if that needs cleanup it can come later. > > Reviewed-by: Eric Sandeen <sandeen@redhat.com> Thanks. --D > > --- > > fs/xfs/libxfs/xfs_bmap.c | 2 +- > > fs/xfs/libxfs/xfs_dir2.c | 21 +++++++++++++++++++++ > > fs/xfs/libxfs/xfs_dir2_priv.h | 29 +++++++++-------------------- > > fs/xfs/libxfs/xfs_dir2_sf.c | 6 +++--- > > 4 files changed, 34 insertions(+), 24 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > > index 4a802b3abe77..4c2e046fbfad 100644 > > --- a/fs/xfs/libxfs/xfs_bmap.c > > +++ b/fs/xfs/libxfs/xfs_bmap.c > > @@ -4561,7 +4561,7 @@ xfs_bmapi_convert_delalloc( > > struct xfs_mount *mp = ip->i_mount; > > xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset); > > struct xfs_bmalloca bma = { NULL }; > > - u16 flags = 0; > > + uint16_t flags = 0; > > struct xfs_trans *tp; > > int error; > > > > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c > > index 0aa87cbde49e..dd6fcaaea318 100644 > > --- a/fs/xfs/libxfs/xfs_dir2.c > > +++ b/fs/xfs/libxfs/xfs_dir2.c > > @@ -724,3 +724,24 @@ xfs_dir2_namecheck( > > /* There shouldn't be any slashes or nulls here */ > > return !memchr(name, '/', length) && !memchr(name, 0, length); > > } > > + > > +xfs_dahash_t > > +xfs_dir2_hashname( > > + struct xfs_mount *mp, > > + struct xfs_name *name) > > +{ > > + if (unlikely(xfs_sb_version_hasasciici(&mp->m_sb))) > > + return xfs_ascii_ci_hashname(name); > > + return xfs_da_hashname(name->name, name->len); > > +} > > + > > +enum xfs_dacmp > > +xfs_dir2_compname( > > + struct xfs_da_args *args, > > + const unsigned char *name, > > + int len) > > +{ > > + if (unlikely(xfs_sb_version_hasasciici(&args->dp->i_mount->m_sb))) > > + return xfs_ascii_ci_compname(args, name, len); > > + return xfs_da_compname(args, name, len); > > +} > > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h > > index c031c53d0f0d..01ee0b926572 100644 > > --- a/fs/xfs/libxfs/xfs_dir2_priv.h > > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h > > @@ -175,6 +175,12 @@ extern int xfs_dir2_sf_lookup(struct xfs_da_args *args); > > extern int xfs_dir2_sf_removename(struct xfs_da_args *args); > > extern int xfs_dir2_sf_replace(struct xfs_da_args *args); > > extern xfs_failaddr_t xfs_dir2_sf_verify(struct xfs_inode *ip); > > +int xfs_dir2_sf_entsize(struct xfs_mount *mp, > > + struct xfs_dir2_sf_hdr *hdr, int len); > > +void xfs_dir2_sf_put_ino(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *hdr, > > + struct xfs_dir2_sf_entry *sfep, xfs_ino_t ino); > > +void xfs_dir2_sf_put_ftype(struct xfs_mount *mp, > > + struct xfs_dir2_sf_entry *sfep, uint8_t ftype); > > > > /* xfs_dir2_readdir.c */ > > extern int xfs_readdir(struct xfs_trans *tp, struct xfs_inode *dp, > > @@ -194,25 +200,8 @@ xfs_dir2_data_entsize( > > return round_up(len, XFS_DIR2_DATA_ALIGN); > > } > > > > -static inline xfs_dahash_t > > -xfs_dir2_hashname( > > - struct xfs_mount *mp, > > - struct xfs_name *name) > > -{ > > - if (unlikely(xfs_sb_version_hasasciici(&mp->m_sb))) > > - return xfs_ascii_ci_hashname(name); > > - return xfs_da_hashname(name->name, name->len); > > -} > > - > > -static inline enum xfs_dacmp > > -xfs_dir2_compname( > > - struct xfs_da_args *args, > > - const unsigned char *name, > > - int len) > > -{ > > - if (unlikely(xfs_sb_version_hasasciici(&args->dp->i_mount->m_sb))) > > - return xfs_ascii_ci_compname(args, name, len); > > - return xfs_da_compname(args, name, len); > > -} > > +xfs_dahash_t xfs_dir2_hashname(struct xfs_mount *mp, struct xfs_name *name); > > +enum xfs_dacmp xfs_dir2_compname(struct xfs_da_args *args, > > + const unsigned char *name, int len); > > > > #endif /* __XFS_DIR2_PRIV_H__ */ > > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c > > index 8b94d33d232f..7b7f6fb2ea3b 100644 > > --- a/fs/xfs/libxfs/xfs_dir2_sf.c > > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c > > @@ -37,7 +37,7 @@ static void xfs_dir2_sf_check(xfs_da_args_t *args); > > static void xfs_dir2_sf_toino4(xfs_da_args_t *args); > > static void xfs_dir2_sf_toino8(xfs_da_args_t *args); > > > > -static int > > +int > > xfs_dir2_sf_entsize( > > struct xfs_mount *mp, > > struct xfs_dir2_sf_hdr *hdr, > > @@ -84,7 +84,7 @@ xfs_dir2_sf_get_ino( > > return get_unaligned_be64(from) & XFS_MAXINUMBER; > > } > > > > -static void > > +void > > xfs_dir2_sf_put_ino( > > struct xfs_mount *mp, > > struct xfs_dir2_sf_hdr *hdr, > > @@ -145,7 +145,7 @@ xfs_dir2_sf_get_ftype( > > return XFS_DIR3_FT_UNKNOWN; > > } > > > > -static void > > +void > > xfs_dir2_sf_put_ftype( > > struct xfs_mount *mp, > > struct xfs_dir2_sf_entry *sfep, > >
On Mon, Dec 16, 2019 at 06:35:35PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Prepare to resync the userspace libxfs with the kernel libxfs. There > were a few things I missed -- a couple of static inline directory > functions that have to be exported for xfs_repair; a couple of directory > naming functions that make porting much easier if they're /not/ static > inline; and a u16 usage that should have been uint16_t. > > None of these things are bugs in their own right; this just makes > porting xfsprogs easier. Instead of exporting random low-level helpers can you please look into refactoring repair to not require such low level access. E.g. the put_ino helper seems to be mostly used for convert short form directories from and to the 8 byte inode format, for which we already have kernel helpers in a slighty different form. I'm also kinda pissed that this was rushed into mainline after -rc2 despite not fixing anything in the kernel. That is not how the development cycle is supposed to work.
On Tue, Dec 24, 2019 at 12:29:54AM -0800, Christoph Hellwig wrote: > On Mon, Dec 16, 2019 at 06:35:35PM -0800, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Prepare to resync the userspace libxfs with the kernel libxfs. There > > were a few things I missed -- a couple of static inline directory > > functions that have to be exported for xfs_repair; a couple of directory > > naming functions that make porting much easier if they're /not/ static > > inline; and a u16 usage that should have been uint16_t. > > > > None of these things are bugs in their own right; this just makes > > porting xfsprogs easier. > > Instead of exporting random low-level helpers can you please look > into refactoring repair to not require such low level access. E.g. > the put_ino helper seems to be mostly used for convert short form > directories from and to the 8 byte inode format, for which we already > have kernel helpers in a slighty different form. We do? I didn't find /any/ helpers to fix shortform inums and ftype. xfs_repair directly manipulates a lot of directory structures directly with libxfs functions. > I'm also kinda pissed that this was rushed into mainline after -rc2 > despite not fixing anything in the kernel. That is not how the > development cycle is supposed to work. Frankly, I trust you (and everyone else) to know when a kernel patchset heading upstream is going to require a lot of non-trivial changes to xfsprogs and to send a patchset to make whatever cleanups to xfsprogs are necessary and then port the kernel patches. 95% of the time, the API changes are pretty minor, and Eric and I can just figure it out between the two of us. The directory refactoring you sent for 5.5 turned out to involve a lot more work and I didn't want Eric to be 100% stuck with the burden of figuring out how to apply everything. But maybe I should start asking submitters always to send kernel + xfsprogs patches at the same time. Allison's been doing that with the deferred attr patches and it's very helpful not to have to imagine what the userspace changes will look like. So anyway, I am sorry for ruffling your feathers. I am particularly bad at handling small cleanups to smooth over xfsprogs when reviewers are short. --D
On Tue, Dec 24, 2019 at 01:27:24PM -0800, Darrick J. Wong wrote: > > Instead of exporting random low-level helpers can you please look > > into refactoring repair to not require such low level access. E.g. > > the put_ino helper seems to be mostly used for convert short form > > directories from and to the 8 byte inode format, for which we already > > have kernel helpers in a slighty different form. > > We do? I didn't find /any/ helpers to fix shortform inums and ftype. > xfs_repair directly manipulates a lot of directory structures directly > with libxfs functions. We have helpers to convert between the 4 and 7 byte ino sf format, which sounds like something that should be reused. > So anyway, I am sorry for ruffling your feathers. I am particularly bad > at handling small cleanups to smooth over xfsprogs when reviewers are > short. What really annoys me is not that patch - it is worth a discussion. The problem is that you rushed it into -rc against the merge window rules before we could even have a discussion.
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 4a802b3abe77..4c2e046fbfad 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -4561,7 +4561,7 @@ xfs_bmapi_convert_delalloc( struct xfs_mount *mp = ip->i_mount; xfs_fileoff_t offset_fsb = XFS_B_TO_FSBT(mp, offset); struct xfs_bmalloca bma = { NULL }; - u16 flags = 0; + uint16_t flags = 0; struct xfs_trans *tp; int error; diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c index 0aa87cbde49e..dd6fcaaea318 100644 --- a/fs/xfs/libxfs/xfs_dir2.c +++ b/fs/xfs/libxfs/xfs_dir2.c @@ -724,3 +724,24 @@ xfs_dir2_namecheck( /* There shouldn't be any slashes or nulls here */ return !memchr(name, '/', length) && !memchr(name, 0, length); } + +xfs_dahash_t +xfs_dir2_hashname( + struct xfs_mount *mp, + struct xfs_name *name) +{ + if (unlikely(xfs_sb_version_hasasciici(&mp->m_sb))) + return xfs_ascii_ci_hashname(name); + return xfs_da_hashname(name->name, name->len); +} + +enum xfs_dacmp +xfs_dir2_compname( + struct xfs_da_args *args, + const unsigned char *name, + int len) +{ + if (unlikely(xfs_sb_version_hasasciici(&args->dp->i_mount->m_sb))) + return xfs_ascii_ci_compname(args, name, len); + return xfs_da_compname(args, name, len); +} diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h index c031c53d0f0d..01ee0b926572 100644 --- a/fs/xfs/libxfs/xfs_dir2_priv.h +++ b/fs/xfs/libxfs/xfs_dir2_priv.h @@ -175,6 +175,12 @@ extern int xfs_dir2_sf_lookup(struct xfs_da_args *args); extern int xfs_dir2_sf_removename(struct xfs_da_args *args); extern int xfs_dir2_sf_replace(struct xfs_da_args *args); extern xfs_failaddr_t xfs_dir2_sf_verify(struct xfs_inode *ip); +int xfs_dir2_sf_entsize(struct xfs_mount *mp, + struct xfs_dir2_sf_hdr *hdr, int len); +void xfs_dir2_sf_put_ino(struct xfs_mount *mp, struct xfs_dir2_sf_hdr *hdr, + struct xfs_dir2_sf_entry *sfep, xfs_ino_t ino); +void xfs_dir2_sf_put_ftype(struct xfs_mount *mp, + struct xfs_dir2_sf_entry *sfep, uint8_t ftype); /* xfs_dir2_readdir.c */ extern int xfs_readdir(struct xfs_trans *tp, struct xfs_inode *dp, @@ -194,25 +200,8 @@ xfs_dir2_data_entsize( return round_up(len, XFS_DIR2_DATA_ALIGN); } -static inline xfs_dahash_t -xfs_dir2_hashname( - struct xfs_mount *mp, - struct xfs_name *name) -{ - if (unlikely(xfs_sb_version_hasasciici(&mp->m_sb))) - return xfs_ascii_ci_hashname(name); - return xfs_da_hashname(name->name, name->len); -} - -static inline enum xfs_dacmp -xfs_dir2_compname( - struct xfs_da_args *args, - const unsigned char *name, - int len) -{ - if (unlikely(xfs_sb_version_hasasciici(&args->dp->i_mount->m_sb))) - return xfs_ascii_ci_compname(args, name, len); - return xfs_da_compname(args, name, len); -} +xfs_dahash_t xfs_dir2_hashname(struct xfs_mount *mp, struct xfs_name *name); +enum xfs_dacmp xfs_dir2_compname(struct xfs_da_args *args, + const unsigned char *name, int len); #endif /* __XFS_DIR2_PRIV_H__ */ diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c index 8b94d33d232f..7b7f6fb2ea3b 100644 --- a/fs/xfs/libxfs/xfs_dir2_sf.c +++ b/fs/xfs/libxfs/xfs_dir2_sf.c @@ -37,7 +37,7 @@ static void xfs_dir2_sf_check(xfs_da_args_t *args); static void xfs_dir2_sf_toino4(xfs_da_args_t *args); static void xfs_dir2_sf_toino8(xfs_da_args_t *args); -static int +int xfs_dir2_sf_entsize( struct xfs_mount *mp, struct xfs_dir2_sf_hdr *hdr, @@ -84,7 +84,7 @@ xfs_dir2_sf_get_ino( return get_unaligned_be64(from) & XFS_MAXINUMBER; } -static void +void xfs_dir2_sf_put_ino( struct xfs_mount *mp, struct xfs_dir2_sf_hdr *hdr, @@ -145,7 +145,7 @@ xfs_dir2_sf_get_ftype( return XFS_DIR3_FT_UNKNOWN; } -static void +void xfs_dir2_sf_put_ftype( struct xfs_mount *mp, struct xfs_dir2_sf_entry *sfep,