Review: factor extracting extent size hints from the inode
Replace frequently repeated, open coded extraction of the extent size hint from the xfs_inode with a single helper function.
Cheers,
Dave.
Signature · 214 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
---
fs/xfs/xfs_bmap.c | 33 +++++++++++----------------------
fs/xfs/xfs_iomap.c | 19 ++++---------------
fs/xfs/xfs_rw.h | 22 ++++++++++++++++++++++
fs/xfs/xfs_vnodeops.c | 17 +++++------------
4 files changed, 42 insertions(+), 49 deletions(-)
Index: 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_vnodeops.c 2007-05-31 17:07:38.421796043 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c 2007-05-31 17:14:29.188303231 +1000
@@ -197,9 +197,8 @@ xfs_getattr(
* realtime extent size or the realtime volume's
* extent size.
*/
- vap->va_blocksize = ip->i_d.di_extsize ?
- (ip->i_d.di_extsize << mp->m_sb.sb_blocklog) :
- (mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog);
+ vap->va_blocksize =
+ xfs_get_extsz_hint(ip) << mp->m_sb.sb_blocklog;
}
break;
}
@@ -4094,22 +4093,16 @@ xfs_alloc_file_space(
if (XFS_FORCED_SHUTDOWN(mp))
return XFS_ERROR(EIO);
- rt = XFS_IS_REALTIME_INODE(ip);
- if (unlikely(rt)) {
- if (!(extsz = ip->i_d.di_extsize))
- extsz = mp->m_sb.sb_rextsize;
- } else {
- extsz = ip->i_d.di_extsize;
- }
-
if ((error = XFS_QM_DQATTACH(mp, ip, 0)))
return error;
if (len <= 0)
return XFS_ERROR(EINVAL);
+ rt = XFS_IS_REALTIME_INODE(ip);
+ extsz = xfs_get_extsz_hint(ip);
+
count = len;
- error = 0;
imapp = &imaps[0];
nimaps = 1;
bmapi_flag = XFS_BMAPI_WRITE | (alloc_type ? XFS_BMAPI_PREALLOC : 0);
Index: 2.6.x-xfs-new/fs/xfs/xfs_rw.h
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_rw.h 2007-05-31 17:07:37.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_rw.h 2007-05-31 17:36:31.711921349 +1000
@@ -77,6 +77,28 @@ xfs_fsb_to_db_io(struct xfs_iocore *io,
#define XFS_FREE_EOF_LOCK (1<<0)
#define XFS_FREE_EOF_NOLOCK (1<<1)
+
+/*
+ * helper function to extract extent size hint from inode
+ */
+STATIC_INLINE xfs_extlen_t
+xfs_get_extsz_hint(
+ xfs_inode_t *ip)
+{
+ xfs_extlen_t extsz;
+
+ if (unlikely(ip->i_d.di_flags & XFS_DIFLAG_REALTIME)) {
+ extsz = (ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)
+ ? ip->i_d.di_extsize
+ : ip->i_mount->m_sb.sb_rextsize;
+ ASSERT(extsz);
+ } else {
+ extsz = (ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)
+ ? ip->i_d.di_extsize : 0;
+ }
+ return extsz;
+}
+
/*
* Prototypes for functions in xfs_rw.c.
*/
Index: 2.6.x-xfs-new/fs/xfs/xfs_bmap.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_bmap.c 2007-05-29 16:40:12.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_bmap.c 2007-05-31 17:38:24.429227867 +1000
@@ -2618,8 +2618,7 @@ xfs_bmap_rtalloc(
xfs_rtblock_t rtb;
mp = ap->ip->i_mount;
- align = ap->ip->i_d.di_extsize ?
- ap->ip->i_d.di_extsize : mp->m_sb.sb_rextsize;
+ align = xfs_get_extsz_hint(ap->ip);
prod = align / mp->m_sb.sb_rextsize;
error = xfs_bmap_extsize_align(mp, ap->gotp, ap->prevp,
align, 1, ap->eof, 0,
@@ -2727,9 +2726,7 @@ xfs_bmap_btalloc(
if (!args)
return XFS_ERROR(ENOMEM);
mp = ap->ip->i_mount;
- align = (ap->userdata && ap->ip->i_d.di_extsize &&
- (ap->ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)) ?
- ap->ip->i_d.di_extsize : 0;
+ align = ap->userdata ? xfs_get_extsz_hint(ap->ip) : 0;
if (unlikely(align)) {
error = xfs_bmap_extsize_align(mp, ap->gotp, ap->prevp,
align, 0, ap->eof, 0, ap->conv,
@@ -2829,9 +2826,9 @@ xfs_bmap_btalloc(
args->total = ap->total;
args->minlen = ap->minlen;
}
- if (unlikely(ap->userdata && ap->ip->i_d.di_extsize &&
- (ap->ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE))) {
- args->prod = ap->ip->i_d.di_extsize;
+ /* apply extent size hints if obtained earlier */
+ if (unlikely(align)) {
+ args->prod = align;
if ((args->mod = (xfs_extlen_t)do_mod(ap->off, args->prod)))
args->mod = (xfs_extlen_t)(args->prod - args->mod);
} else if (mp->m_sb.sb_blocksize >= NBPP) {
@@ -3018,9 +3015,7 @@ xfs_bmap_filestreams(
*/
mp = ap->ip->i_mount;
rt = (ap->ip->i_d.di_flags & XFS_DIFLAG_REALTIME) && ap->userdata;
- align = (ap->userdata && ap->ip->i_d.di_extsize &&
- (ap->ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)) ?
- ap->ip->i_d.di_extsize : 0;
+ align = ap->userdata ? xfs_get_extsz_hint(ap->ip) : 0;
if (align) {
error = xfs_bmap_extsize_align(mp, ap->gotp, ap->prevp,
align, rt,
@@ -3166,9 +3161,9 @@ xfs_bmap_filestreams(
args.total = ap->total;
args.minlen = ap->minlen;
}
- if (ap->userdata && ap->ip->i_d.di_extsize &&
- (ap->ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)) {
- args.prod = ap->ip->i_d.di_extsize;
+ /* apply extent size hint if found earlier */
+ if (align) {
+ args.prod = align;
if ((args.mod = (xfs_extlen_t)(do_mod(ap->off, args.prod))))
args.mod = (xfs_extlen_t)(args.prod - args.mod);
} else if (mp->m_sb.sb_blocksize >= NBPP) {
@@ -5224,12 +5219,7 @@ xfs_bmapi(
xfs_extlen_t extsz;
/* Figure out the extent size, adjust alen */
- if (rt) {
- if (!(extsz = ip->i_d.di_extsize))
- extsz = mp->m_sb.sb_rextsize;
- } else {
- extsz = ip->i_d.di_extsize;
- }
+ extsz = xfs_get_extsz_hint(ip);
if (extsz) {
error = xfs_bmap_extsize_align(mp,
&got, &prev, extsz,
@@ -6170,8 +6160,7 @@ xfs_getbmap(
ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
return XFS_ERROR(EINVAL);
if (whichfork == XFS_DATA_FORK) {
- if ((ip->i_d.di_extsize && (ip->i_d.di_flags &
- (XFS_DIFLAG_REALTIME|XFS_DIFLAG_EXTSIZE))) ||
+ if (xfs_get_extsz_hint(ip) ||
ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC|XFS_DIFLAG_APPEND)){
prealloced = 1;
fixlen = XFS_MAXIOFFSET(mp);
Index: 2.6.x-xfs-new/fs/xfs/xfs_iomap.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_iomap.c 2007-05-31 17:07:38.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_iomap.c 2007-05-31 17:30:22.096110172 +1000
@@ -451,19 +451,14 @@ xfs_iomap_write_direct(
return XFS_ERROR(error);
rt = XFS_IS_REALTIME_INODE(ip);
- if (unlikely(rt)) {
- if (!(extsz = ip->i_d.di_extsize))
- extsz = mp->m_sb.sb_rextsize;
- } else {
- extsz = ip->i_d.di_extsize;
- }
+ extsz = xfs_get_extsz_hint(ip);
isize = ip->i_size;
if (io->io_new_size > isize)
isize = io->io_new_size;
- offset_fsb = XFS_B_TO_FSBT(mp, offset);
- last_fsb = XFS_B_TO_FSB(mp, ((xfs_ufsize_t)(offset + count)));
+ offset_fsb = XFS_B_TO_FSBT(mp, offset);
+ last_fsb = XFS_B_TO_FSB(mp, ((xfs_ufsize_t)(offset + count)));
if ((offset + count) > isize) {
error = xfs_iomap_eof_align_last_fsb(mp, io, isize, extsz,
&last_fsb);
@@ -666,13 +661,7 @@ xfs_iomap_write_delay(
if (error)
return XFS_ERROR(error);
- if (XFS_IS_REALTIME_INODE(ip)) {
- if (!(extsz = ip->i_d.di_extsize))
- extsz = mp->m_sb.sb_rextsize;
- } else {
- extsz = ip->i_d.di_extsize;
- }
-
+ extsz = xfs_get_extsz_hint(ip);
offset_fsb = XFS_B_TO_FSBT(mp, offset);
retry:Quoted 3 lines · David Chinner
Replace frequently repeated, open coded extraction of the extent size hint from the
xfs_inodewith a single helper function.
Looks good, but I'd suggest not putting in the unlikelys. Realtime or alignment are perfectly normal codepaths and hardcoding them to be predicted not taken sounds like a bad idea. unlilely should be limited to exception error handling code.
Quoted 8 lines · David Chinner
Replace frequently repeated, open coded extraction of the extent size hint from the
xfs_inodewith a single helper function.Cheers,
Dave.
Dave,
I think XFS_DIFLAG_REALTIME and XFS_DIFLAG_EXTSIZE flags are mutually exclusive.XFS_DIFLAG_REALTIME and di_extsize have been introduced and used on Irix and Linux before XFS_DIFLAG_EXTSIZE.
This code:
+ if (unlikely(ip->i_d.di_flags & XFS_DIFLAG_REALTIME)) {
+ extsz = (ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE)
+ ? ip->i_d.di_extsize
+ : ip->i_mount->m_sb.sb_rextsize;
+ ASSERT(extsz);
+ } else {
shouldn't test for XFS_DIFLAG_EXTSIZE but use di_extsize if non zero.
Regards,
Vlad
Quoted 5 lines · Vlad Apostolov
Quoted 4 lines · David Chinner
Replace frequently repeated, open coded extraction of the extent size hint from the
xfs_inodewith a single helper function.
Quoted 8 lines
Quoted 4 lines
Cheers,
Dave.
Dave,
I think
XFS_DIFLAG_REALTIMEandXFS_DIFLAG_EXTSIZEflags are mutually exclusive.
No, that's not true.
Quoted 2 lines
XFS_DIFLAG_REALTIMEanddi_extsizehave been introduced and used on Irix and Linux beforeXFS_DIFLAG_EXTSIZE.
Sure, but look at how we are supposed to set the extent size hint. i.e XFS_IOC_FSSETXATTR, which by your own account should have the XFS_XFLAG_EXTSIZE bit set in it. And that does not matter if the file is realtime or not.
See the xfs_io code that sets the extent size hint:
567 if (S_ISREG(stat.st_mode)) {
568 fsx.fsx_xflags |= XFS_XFLAG_EXTSIZE;
569 } else if (S_ISDIR(stat.st_mode)) {
570 fsx.fsx_xflags |= XFS_XFLAG_EXTSZINHERIT;
571 } else {
572 printf(_("invalid target file type - file %s\n"), path);
573 return 0;
574 }
575 fsx.fsx_extsize = extsz;
576
577 if ((xfsctl(path, fd, XFS_IOC_FSSETXATTR, &fsx)) < 0) {
578 printf("%s: XFS_IOC_FSSETXATTR %s: %s\n",
579 progname, path, strerror(errno));
580 return 0;
581 }If you use this method of setting the extent size hint, then you will *always* get the XFS_DIFLAG_EXTSIZE flag set when you have an extent size hint, regardless of whether it is a realtime file or not.
Quoted 10 lines
This code:
(quoted diff) +6
-0+ if (unlikely(ip->i_d.di_flags & XFS_DIFLAG_REALTIME)) { + extsz = (ip->i_d.di_flags & XFS_DIFLAG_EXTSIZE) + ? ip->i_d.di_extsize + : ip->i_mount->m_sb.sb_rextsize; + ASSERT(extsz); + } else {shouldn't test for
XFS_DIFLAG_EXTSIZEbut usedi_extsizeif non zero.
Hmmmm - I think having one rule for realtime and a different rule for data to do exactly the same thing is busted.
Like the realtime code, there are many places the non-realtime code don't bother to check for a valid extent size hint flag - it just read it blindly. That was part of the problem that the DMF folks tripped over - the flag wasn't set but the hint was, and bad things were happening because it wasn't consistently applied.
Also, if we want to fix up the setting of the extent size hint so that it errors out if you don't set the XFS_XFLAG_EXTSIZE as well, then we'd have to make an exception for the realtime inodes.
So, there's plenty of reasons for leaving this code as it stands and have both realtime and non-realtime behave the same way.
Thoughts?
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group
Quoted 4 lines · David Chinner
If you use this method of setting the extent size hint, then you will *always* get the
XFS_DIFLAG_EXTSIZEflag set when you have an extent size hint, regardless of whether it is a realtime file or not.
The extsize flag is relatively recent though, and traditionally realtime files could have had their extsize explicitly set with no associated extsize flag (thats just how it was implemented, originally, in realtime).
But, not many people use realtime, even fewer would be using the extent size option with realtime (like, none?, on Linux anyway) ... so, you could pretty much make whatever rule you like.
cheers.
Signature · 1 line
Nathan
Quoted 10 lines · Nathan Scott
Quoted 4 lines · David Chinner
If you use this method of setting the extent size hint, then you will *always* get the
XFS_DIFLAG_EXTSIZEflag set when you have an extent size hint, regardless of whether it is a realtime file or not.The extsize flag is relatively recent though, and traditionally realtime files could have had their extsize explicitly set with no associated extsize flag (thats just how it was implemented, originally, in realtime).
*nod*
We've got recent bugs reported because of this assumption and lack of checking of the extent size hint flag where it needs to be checked.
Either we have a flag to indicate the di_extsize field is valid or we don't - it's too confusing to have different interfaces just because an inode has a different, unrelated flag set on it. Now that we have a flag, we can't remove support for it.....
Quoted 3 lines
But, not many people use realtime, even fewer would be using the extent size option with realtime (like, none?, on Linux anyway) ... so, you could pretty much make whatever rule you like.
I sorta left that unsaid, but that is yet another reason I think the change should stand.
Cheers,
Dave.
Signature · 3 lines
Dave Chinner Principal Engineer SGI Australian Software Group