* [PATCHSET 0/3] xfs: various unit conversion
@ 2021-05-31 22:40 Darrick J. Wong
2021-05-31 22:40 ` [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags Darrick J. Wong
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Darrick J. Wong @ 2021-05-31 22:40 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, hch
Hi all,
Crafting the realtime file extent size hint fixes revealed various
opportunities to clean up unit conversions, so now that gets its own
series.
If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.
This is an extraordinary way to destroy everything. Enjoy!
Comments and questions are, as always, welcome.
--D
kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=unit-conversion-cleanups-5.14
---
fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
fs/xfs/xfs_bmap_util.c | 6 +++---
fs/xfs/xfs_inode.c | 25 +++++++++++--------------
fs/xfs/xfs_iops.c | 4 ++--
4 files changed, 17 insertions(+), 20 deletions(-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags
2021-05-31 22:40 [PATCHSET 0/3] xfs: various unit conversion Darrick J. Wong
@ 2021-05-31 22:40 ` Darrick J. Wong
2021-05-31 23:33 ` Dave Chinner
2021-05-31 22:40 ` [PATCH 2/3] xfs: clean up open-coded fs block unit conversions Darrick J. Wong
2021-05-31 22:40 ` [PATCH 3/3] xfs: remove unnecessary shifts Darrick J. Wong
2 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2021-05-31 22:40 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, hch
From: Darrick J. Wong <djwong@kernel.org>
Remove the unnecessary convenience variable.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/xfs_inode.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index e4c2da4566f1..1e28997c6f78 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -689,47 +689,44 @@ xfs_inode_inherit_flags(
struct xfs_inode *ip,
const struct xfs_inode *pip)
{
- unsigned int di_flags = 0;
xfs_failaddr_t failaddr;
umode_t mode = VFS_I(ip)->i_mode;
if (S_ISDIR(mode)) {
if (pip->i_diflags & XFS_DIFLAG_RTINHERIT)
- di_flags |= XFS_DIFLAG_RTINHERIT;
+ ip->i_diflags |= XFS_DIFLAG_RTINHERIT;
if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
- di_flags |= XFS_DIFLAG_EXTSZINHERIT;
+ ip->i_diflags |= XFS_DIFLAG_EXTSZINHERIT;
ip->i_extsize = pip->i_extsize;
}
if (pip->i_diflags & XFS_DIFLAG_PROJINHERIT)
- di_flags |= XFS_DIFLAG_PROJINHERIT;
+ ip->i_diflags |= XFS_DIFLAG_PROJINHERIT;
} else if (S_ISREG(mode)) {
if ((pip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
xfs_sb_version_hasrealtime(&ip->i_mount->m_sb))
- di_flags |= XFS_DIFLAG_REALTIME;
+ ip->i_diflags |= XFS_DIFLAG_REALTIME;
if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
- di_flags |= XFS_DIFLAG_EXTSIZE;
+ ip->i_diflags |= XFS_DIFLAG_EXTSIZE;
ip->i_extsize = pip->i_extsize;
}
}
if ((pip->i_diflags & XFS_DIFLAG_NOATIME) &&
xfs_inherit_noatime)
- di_flags |= XFS_DIFLAG_NOATIME;
+ ip->i_diflags |= XFS_DIFLAG_NOATIME;
if ((pip->i_diflags & XFS_DIFLAG_NODUMP) &&
xfs_inherit_nodump)
- di_flags |= XFS_DIFLAG_NODUMP;
+ ip->i_diflags |= XFS_DIFLAG_NODUMP;
if ((pip->i_diflags & XFS_DIFLAG_SYNC) &&
xfs_inherit_sync)
- di_flags |= XFS_DIFLAG_SYNC;
+ ip->i_diflags |= XFS_DIFLAG_SYNC;
if ((pip->i_diflags & XFS_DIFLAG_NOSYMLINKS) &&
xfs_inherit_nosymlinks)
- di_flags |= XFS_DIFLAG_NOSYMLINKS;
+ ip->i_diflags |= XFS_DIFLAG_NOSYMLINKS;
if ((pip->i_diflags & XFS_DIFLAG_NODEFRAG) &&
xfs_inherit_nodefrag)
- di_flags |= XFS_DIFLAG_NODEFRAG;
+ ip->i_diflags |= XFS_DIFLAG_NODEFRAG;
if (pip->i_diflags & XFS_DIFLAG_FILESTREAM)
- di_flags |= XFS_DIFLAG_FILESTREAM;
-
- ip->i_diflags |= di_flags;
+ ip->i_diflags |= XFS_DIFLAG_FILESTREAM;
/*
* Inode verifiers on older kernels only check that the extent size
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] xfs: clean up open-coded fs block unit conversions
2021-05-31 22:40 [PATCHSET 0/3] xfs: various unit conversion Darrick J. Wong
2021-05-31 22:40 ` [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags Darrick J. Wong
@ 2021-05-31 22:40 ` Darrick J. Wong
2021-05-31 23:33 ` Dave Chinner
2021-06-01 10:43 ` Carlos Maiolino
2021-05-31 22:40 ` [PATCH 3/3] xfs: remove unnecessary shifts Darrick J. Wong
2 siblings, 2 replies; 11+ messages in thread
From: Darrick J. Wong @ 2021-05-31 22:40 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, hch
From: Darrick J. Wong <djwong@kernel.org>
Replace some open-coded fs block unit conversions with the standard
conversion macro.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
fs/xfs/xfs_iops.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index f3254a4f4cb4..04ce361688f7 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -612,7 +612,7 @@ xfs_inode_validate_extsize(
*/
if (rt_flag)
- blocksize_bytes = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
+ blocksize_bytes = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize);
else
blocksize_bytes = mp->m_sb.sb_blocksize;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index dfe24b7f26e5..93c082db04b7 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -543,7 +543,7 @@ xfs_stat_blksize(
* always return the realtime extent size.
*/
if (XFS_IS_REALTIME_INODE(ip))
- return xfs_get_extsz_hint(ip) << mp->m_sb.sb_blocklog;
+ return XFS_FSB_TO_B(mp, xfs_get_extsz_hint(ip));
/*
* Allow large block sizes to be reported to userspace programs if the
@@ -560,7 +560,7 @@ xfs_stat_blksize(
*/
if (mp->m_flags & XFS_MOUNT_LARGEIO) {
if (mp->m_swidth)
- return mp->m_swidth << mp->m_sb.sb_blocklog;
+ return XFS_FSB_TO_B(mp, mp->m_swidth);
if (mp->m_flags & XFS_MOUNT_ALLOCSIZE)
return 1U << mp->m_allocsize_log;
}
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] xfs: remove unnecessary shifts
2021-05-31 22:40 [PATCHSET 0/3] xfs: various unit conversion Darrick J. Wong
2021-05-31 22:40 ` [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags Darrick J. Wong
2021-05-31 22:40 ` [PATCH 2/3] xfs: clean up open-coded fs block unit conversions Darrick J. Wong
@ 2021-05-31 22:40 ` Darrick J. Wong
2021-05-31 23:34 ` Dave Chinner
2021-06-01 10:44 ` Carlos Maiolino
2 siblings, 2 replies; 11+ messages in thread
From: Darrick J. Wong @ 2021-05-31 22:40 UTC (permalink / raw)
To: djwong; +Cc: linux-xfs, hch
From: Darrick J. Wong <djwong@kernel.org>
The superblock verifier already validates that (1 << blocklog) ==
blocksize, so use the value directly instead of doing math.
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
fs/xfs/xfs_bmap_util.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 0936f3a96fe6..997eb5c6e9b4 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -945,7 +945,7 @@ xfs_flush_unmap_range(
xfs_off_t rounding, start, end;
int error;
- rounding = max_t(xfs_off_t, 1 << mp->m_sb.sb_blocklog, PAGE_SIZE);
+ rounding = max_t(xfs_off_t, mp->m_sb.sb_blocksize, PAGE_SIZE);
start = round_down(offset, rounding);
end = round_up(offset + len, rounding) - 1;
@@ -1053,9 +1053,9 @@ xfs_prepare_shift(
* extent (after split) during the shift and corrupt the file. Start
* with the block just prior to the start to stabilize the boundary.
*/
- offset = round_down(offset, 1 << mp->m_sb.sb_blocklog);
+ offset = round_down(offset, mp->m_sb.sb_blocksize);
if (offset)
- offset -= (1 << mp->m_sb.sb_blocklog);
+ offset -= mp->m_sb.sb_blocksize;
/*
* Writeback and invalidate cache for the remainder of the file as we're
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags
2021-05-31 22:40 ` [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags Darrick J. Wong
@ 2021-05-31 23:33 ` Dave Chinner
2021-06-01 10:43 ` Carlos Maiolino
2021-06-01 18:52 ` Darrick J. Wong
0 siblings, 2 replies; 11+ messages in thread
From: Dave Chinner @ 2021-05-31 23:33 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
On Mon, May 31, 2021 at 03:40:38PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Remove the unnecessary convenience variable.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> fs/xfs/xfs_inode.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index e4c2da4566f1..1e28997c6f78 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -689,47 +689,44 @@ xfs_inode_inherit_flags(
> struct xfs_inode *ip,
> const struct xfs_inode *pip)
> {
> - unsigned int di_flags = 0;
> xfs_failaddr_t failaddr;
> umode_t mode = VFS_I(ip)->i_mode;
>
> if (S_ISDIR(mode)) {
> if (pip->i_diflags & XFS_DIFLAG_RTINHERIT)
> - di_flags |= XFS_DIFLAG_RTINHERIT;
> + ip->i_diflags |= XFS_DIFLAG_RTINHERIT;
> if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
> - di_flags |= XFS_DIFLAG_EXTSZINHERIT;
> + ip->i_diflags |= XFS_DIFLAG_EXTSZINHERIT;
> ip->i_extsize = pip->i_extsize;
> }
Hmmmm.
IIRC, these functions were originally written this way because the
compiler generated much better code using a local variable than when
writing directly to the ip->i_d.di_flags. Is this still true now?
I think it's worth checking, because we have changed the structure
of the xfs_inode (removed the i_d structure) so maybe this isn't a
concern anymore?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] xfs: clean up open-coded fs block unit conversions
2021-05-31 22:40 ` [PATCH 2/3] xfs: clean up open-coded fs block unit conversions Darrick J. Wong
@ 2021-05-31 23:33 ` Dave Chinner
2021-06-01 10:43 ` Carlos Maiolino
1 sibling, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2021-05-31 23:33 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
On Mon, May 31, 2021 at 03:40:43PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Replace some open-coded fs block unit conversions with the standard
> conversion macro.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
LGTM.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] xfs: remove unnecessary shifts
2021-05-31 22:40 ` [PATCH 3/3] xfs: remove unnecessary shifts Darrick J. Wong
@ 2021-05-31 23:34 ` Dave Chinner
2021-06-01 10:44 ` Carlos Maiolino
1 sibling, 0 replies; 11+ messages in thread
From: Dave Chinner @ 2021-05-31 23:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
On Mon, May 31, 2021 at 03:40:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> The superblock verifier already validates that (1 << blocklog) ==
> blocksize, so use the value directly instead of doing math.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks fine.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags
2021-05-31 23:33 ` Dave Chinner
@ 2021-06-01 10:43 ` Carlos Maiolino
2021-06-01 18:52 ` Darrick J. Wong
1 sibling, 0 replies; 11+ messages in thread
From: Carlos Maiolino @ 2021-06-01 10:43 UTC (permalink / raw)
To: Dave Chinner; +Cc: Darrick J. Wong, linux-xfs, hch
On Tue, Jun 01, 2021 at 09:33:15AM +1000, Dave Chinner wrote:
> On Mon, May 31, 2021 at 03:40:38PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Remove the unnecessary convenience variable.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > fs/xfs/xfs_inode.c | 25 +++++++++++--------------
> > 1 file changed, 11 insertions(+), 14 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index e4c2da4566f1..1e28997c6f78 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -689,47 +689,44 @@ xfs_inode_inherit_flags(
> > struct xfs_inode *ip,
> > const struct xfs_inode *pip)
> > {
> > - unsigned int di_flags = 0;
> > xfs_failaddr_t failaddr;
> > umode_t mode = VFS_I(ip)->i_mode;
> >
> > if (S_ISDIR(mode)) {
> > if (pip->i_diflags & XFS_DIFLAG_RTINHERIT)
> > - di_flags |= XFS_DIFLAG_RTINHERIT;
> > + ip->i_diflags |= XFS_DIFLAG_RTINHERIT;
> > if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
> > - di_flags |= XFS_DIFLAG_EXTSZINHERIT;
> > + ip->i_diflags |= XFS_DIFLAG_EXTSZINHERIT;
> > ip->i_extsize = pip->i_extsize;
> > }
>
> Hmmmm.
>
> IIRC, these functions were originally written this way because the
> compiler generated much better code using a local variable than when
> writing directly to the ip->i_d.di_flags. Is this still true now?
I did a quick look into the generated asm code, and although my asm is a bit
rusty, I don't see that much difference between the two versions, at least not
to describe it as much better code. The following is a snippet of the code with
and without the patch, first line is just as a reference guide from where the
ASM starts. Most of the other occurrences using di_flags variable are similar
if not the same:
Without patch applied:
} else if (S_ISREG(mode)) {
92473: 66 81 f9 00 80 cmp $0x8000,%cx
92478: 0f 84 59 01 00 00 je 925d7 <xfs_dir_ialloc+0x797>
9248e: 41 8b b5 0c 01 00 00 mov 0x10c(%r13),%esi
92485: 31 c9 xor %ecx,%ecx
92487: a8 40 test $0x40,%al
92489: 74 15 je 924a0 <xfs_dir_ialloc+0x660>
9248b: 44 8b 1d 00 00 00 00 mov 0x0(%rip),%r11d # 92492 <xfs_dir_ialloc+0x652>
92492: 41 89 c8 mov %ecx,%r8d
92495: 41 83 c8 40 or $0x40,%r8d
92499: 45 85 db test %r11d,%r11d
9249c: 41 0f 45 c8 cmovne %r8d,%ecx
924a0: a8 80 test $0x80,%al
With patch applied:
} else if (S_ISREG(mode)) {
92483: 66 81 fe 00 80 cmp $0x8000,%si
92488: 0f 84 94 01 00 00 je 92622 <xfs_dir_ialloc+0x7e2>
9248e: 41 8b b5 0c 01 00 00 mov 0x10c(%r13),%esi
92495: a8 40 test $0x40,%al
92497: 74 20 je 924b9 <xfs_dir_ialloc+0x679>
92499: 44 8b 05 00 00 00 00 mov 0x0(%rip),%r8d # 924a0 <xfs_dir_ialloc+0x660>
924a0: 45 85 c0 test %r8d,%r8d
924a3: 74 14 je 924b9 <xfs_dir_ialloc+0x679>
924a5: 83 c9 40 or $0x40,%ecx
924a8: 66 41 89 8d 16 01 00 mov %cx,0x116(%r13)
924af: 00
924b0: 41 0f b7 84 24 16 01 movzwl 0x116(%r12),%eax
924b7: 00 00
924b9: a8 80 test $0x80,%al
Roughly, the main difference here IMHO, is the fact the current code uses xor to
zero out the registers prior test/copying the flags into it, while using the
patch applied, this is replaced by movz instructions since it copies the value's
flag directly from the xfs_inode struct. So, IMHO I don't really see enough
difference to justify dropping this patch.
Anyway, as I mentioned, my ASM is a bit rusty, so take it with a pinch of salt
:), but from my side, you can add:
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
Cheers.
--
Carlos
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] xfs: clean up open-coded fs block unit conversions
2021-05-31 22:40 ` [PATCH 2/3] xfs: clean up open-coded fs block unit conversions Darrick J. Wong
2021-05-31 23:33 ` Dave Chinner
@ 2021-06-01 10:43 ` Carlos Maiolino
1 sibling, 0 replies; 11+ messages in thread
From: Carlos Maiolino @ 2021-06-01 10:43 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
On Mon, May 31, 2021 at 03:40:43PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Replace some open-coded fs block unit conversions with the standard
> conversion macro.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks good.
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
> fs/xfs/xfs_iops.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index f3254a4f4cb4..04ce361688f7 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -612,7 +612,7 @@ xfs_inode_validate_extsize(
> */
>
> if (rt_flag)
> - blocksize_bytes = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
> + blocksize_bytes = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize);
> else
> blocksize_bytes = mp->m_sb.sb_blocksize;
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index dfe24b7f26e5..93c082db04b7 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -543,7 +543,7 @@ xfs_stat_blksize(
> * always return the realtime extent size.
> */
> if (XFS_IS_REALTIME_INODE(ip))
> - return xfs_get_extsz_hint(ip) << mp->m_sb.sb_blocklog;
> + return XFS_FSB_TO_B(mp, xfs_get_extsz_hint(ip));
>
> /*
> * Allow large block sizes to be reported to userspace programs if the
> @@ -560,7 +560,7 @@ xfs_stat_blksize(
> */
> if (mp->m_flags & XFS_MOUNT_LARGEIO) {
> if (mp->m_swidth)
> - return mp->m_swidth << mp->m_sb.sb_blocklog;
> + return XFS_FSB_TO_B(mp, mp->m_swidth);
> if (mp->m_flags & XFS_MOUNT_ALLOCSIZE)
> return 1U << mp->m_allocsize_log;
> }
>
--
Carlos
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] xfs: remove unnecessary shifts
2021-05-31 22:40 ` [PATCH 3/3] xfs: remove unnecessary shifts Darrick J. Wong
2021-05-31 23:34 ` Dave Chinner
@ 2021-06-01 10:44 ` Carlos Maiolino
1 sibling, 0 replies; 11+ messages in thread
From: Carlos Maiolino @ 2021-06-01 10:44 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, hch
On Mon, May 31, 2021 at 03:40:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> The superblock verifier already validates that (1 << blocklog) ==
> blocksize, so use the value directly instead of doing math.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Looks good.
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
> fs/xfs/xfs_bmap_util.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 0936f3a96fe6..997eb5c6e9b4 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -945,7 +945,7 @@ xfs_flush_unmap_range(
> xfs_off_t rounding, start, end;
> int error;
>
> - rounding = max_t(xfs_off_t, 1 << mp->m_sb.sb_blocklog, PAGE_SIZE);
> + rounding = max_t(xfs_off_t, mp->m_sb.sb_blocksize, PAGE_SIZE);
> start = round_down(offset, rounding);
> end = round_up(offset + len, rounding) - 1;
>
> @@ -1053,9 +1053,9 @@ xfs_prepare_shift(
> * extent (after split) during the shift and corrupt the file. Start
> * with the block just prior to the start to stabilize the boundary.
> */
> - offset = round_down(offset, 1 << mp->m_sb.sb_blocklog);
> + offset = round_down(offset, mp->m_sb.sb_blocksize);
> if (offset)
> - offset -= (1 << mp->m_sb.sb_blocklog);
> + offset -= mp->m_sb.sb_blocksize;
>
> /*
> * Writeback and invalidate cache for the remainder of the file as we're
>
--
Carlos
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags
2021-05-31 23:33 ` Dave Chinner
2021-06-01 10:43 ` Carlos Maiolino
@ 2021-06-01 18:52 ` Darrick J. Wong
1 sibling, 0 replies; 11+ messages in thread
From: Darrick J. Wong @ 2021-06-01 18:52 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, hch
On Tue, Jun 01, 2021 at 09:33:15AM +1000, Dave Chinner wrote:
> On Mon, May 31, 2021 at 03:40:38PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Remove the unnecessary convenience variable.
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > fs/xfs/xfs_inode.c | 25 +++++++++++--------------
> > 1 file changed, 11 insertions(+), 14 deletions(-)
> >
> >
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index e4c2da4566f1..1e28997c6f78 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -689,47 +689,44 @@ xfs_inode_inherit_flags(
> > struct xfs_inode *ip,
> > const struct xfs_inode *pip)
> > {
> > - unsigned int di_flags = 0;
> > xfs_failaddr_t failaddr;
> > umode_t mode = VFS_I(ip)->i_mode;
> >
> > if (S_ISDIR(mode)) {
> > if (pip->i_diflags & XFS_DIFLAG_RTINHERIT)
> > - di_flags |= XFS_DIFLAG_RTINHERIT;
> > + ip->i_diflags |= XFS_DIFLAG_RTINHERIT;
> > if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
> > - di_flags |= XFS_DIFLAG_EXTSZINHERIT;
> > + ip->i_diflags |= XFS_DIFLAG_EXTSZINHERIT;
> > ip->i_extsize = pip->i_extsize;
> > }
>
> Hmmmm.
>
> IIRC, these functions were originally written this way because the
> compiler generated much better code using a local variable than when
> writing directly to the ip->i_d.di_flags. Is this still true now?
> I think it's worth checking, because we have changed the structure
> of the xfs_inode (removed the i_d structure) so maybe this isn't a
> concern anymore?
Before the patch, the extszinherit code emitted on my system (gcc 10.2)
looked like this (0x15c is the offset of i_extsize and 0x166 is the
offset of i_diflags):
699 if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
0xffffffffa0375715 <+1909>: test $0x10,%ah
0xffffffffa0375718 <+1912>: jne 0xffffffffa0375759 <xfs_dir_ialloc+1977>
0xffffffffa037571a <+1914>: mov 0x15c(%r13),%esi
700 di_flags |= XFS_DIFLAG_EXTSZINHERIT;
0xffffffffa0375759 <+1977>: mov 0x15c(%r12),%esi
0xffffffffa0375761 <+1985>: or $0x10,%ch
701 ip->i_extsize = pip->i_extsize;
0xffffffffa0375764 <+1988>: mov %esi,0x15c(%r13)
0xffffffffa037576b <+1995>: movzwl 0x166(%r12),%eax
0xffffffffa0375774 <+2004>: jmp 0xffffffffa0375721 <xfs_dir_ialloc+1921>
702 }
703 if (pip->i_diflags & XFS_DIFLAG_PROJINHERIT)
0xffffffffa0375721 <+1921>: mov %ecx,%r8d
0xffffffffa0375724 <+1924>: or $0x200,%r8d
0xffffffffa037572b <+1931>: test $0x2,%ah
0xffffffffa037572e <+1934>: cmovne %r8d,%ecx
0xffffffffa0375732 <+1938>: jmpq 0xffffffffa03755e7 <xfs_dir_ialloc+1607>
704 di_flags |= XFS_DIFLAG_PROJINHERIT;
705 } else if (S_ISREG(mode)) {
With a snippet at the bottom to copy %cx back to the inode:
730 di_flags |= XFS_DIFLAG_FILESTREAM;
731
732 ip->i_diflags |= di_flags;
0xffffffffa0375680 <+1760>: or 0x166(%r13),%cx
0xffffffffa0375688 <+1768>: mov %cx,0x166(%r13)
With the patch applied, that code becomes:
698 if (pip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) {
0xffffffffa0375751 <+1969>: test $0x10,%ah
0xffffffffa0375754 <+1972>: jne 0xffffffffa03757d0 <xfs_dir_ialloc+2096>
0xffffffffa0375756 <+1974>: mov 0x15c(%r13),%esi
699 ip->i_diflags |= XFS_DIFLAG_EXTSZINHERIT;
0xffffffffa03757d0 <+2096>: or $0x10,%ch
0xffffffffa03757d3 <+2099>: mov %cx,0x166(%r13)
700 ip->i_extsize = pip->i_extsize;
0xffffffffa03757db <+2107>: mov 0x15c(%r12),%esi
0xffffffffa03757e3 <+2115>: mov %esi,0x15c(%r13)
0xffffffffa03757ea <+2122>: movzwl 0x166(%r12),%eax
0xffffffffa03757f3 <+2131>: jmpq 0xffffffffa037575d <xfs_dir_ialloc+1981>
0xffffffffa03757f8 <+2136>: callq 0xffffffff81713cb0 <__stack_chk_fail>
0xffffffffa03757fd: nopl (%rax)
701 }
702 if (pip->i_diflags & XFS_DIFLAG_PROJINHERIT)
0xffffffffa037575d <+1981>: test $0x2,%ah
0xffffffffa0375760 <+1984>: je 0xffffffffa03755f2 <xfs_dir_ialloc+1618>
703 ip->i_diflags |= XFS_DIFLAG_PROJINHERIT;
0xffffffffa0375766 <+1990>: or $0x2,%ch
0xffffffffa0375769 <+1993>: mov %cx,0x166(%r13)
0xffffffffa0375771 <+2001>: movzwl 0x166(%r12),%eax
0xffffffffa037577a <+2010>: jmpq 0xffffffffa03755f2 <xfs_dir_ialloc+1618>
704 } else if (S_ISREG(mode)) {
AFAICT the main difference between the two versions now is that the new
version will copy %cx back to ip->i_diflags after every set operation,
and the old version would sometimes use a conditional move to update
%cx instead of a conditional branch.
I suspect the old version /is/ more efficient on x86, but I didn't see
any measurable difference between the two versions. Hm. The old code
was 197 bytes vs. 243 for the new one, so fmeh, I'll drop this because
I'd rather move on to the iwalk pre-cleanup cleanup series.
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-01 18:52 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-31 22:40 [PATCHSET 0/3] xfs: various unit conversion Darrick J. Wong
2021-05-31 22:40 ` [PATCH 1/3] xfs: set ip->i_diflags directly in xfs_inode_inherit_flags Darrick J. Wong
2021-05-31 23:33 ` Dave Chinner
2021-06-01 10:43 ` Carlos Maiolino
2021-06-01 18:52 ` Darrick J. Wong
2021-05-31 22:40 ` [PATCH 2/3] xfs: clean up open-coded fs block unit conversions Darrick J. Wong
2021-05-31 23:33 ` Dave Chinner
2021-06-01 10:43 ` Carlos Maiolino
2021-05-31 22:40 ` [PATCH 3/3] xfs: remove unnecessary shifts Darrick J. Wong
2021-05-31 23:34 ` Dave Chinner
2021-06-01 10:44 ` Carlos Maiolino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.