Review: factor extracting extent size hints from the inode

Review: factor extracting extent size hints from the inode

[David Chinner]

Replace frequently repeated, open coded extraction of the
extent size hint from the xfs_inode with a single helper
function.

Cheers,

Dave.
-- 
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:

Re: Review: factor extracting extent size hints from the inode

[Christoph Hellwig]

On Mon, Jun 04, 2007 at 03:23:33PM +1000, David Chinner wrote:
> Replace frequently repeated, open coded extraction of the
> extent size hint from the xfs_inode with 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.

Re: Review: factor extracting extent size hints from the inode

[Vlad Apostolov]

David Chinner wrote:
> Replace frequently repeated, open coded extraction of the
> extent size hint from the xfs_inode with 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 

Re: Review: factor extracting extent size hints from the inode

[David Chinner]

On Tue, Jun 12, 2007 at 03:13:26PM +1000, Vlad Apostolov wrote:
> David Chinner wrote:
> >Replace frequently repeated, open coded extraction of the
> >extent size hint from the xfs_inode with a single helper
> >function.
> >
> >Cheers,
> >
> >Dave.
> >  
> Dave,
> 
> I think XFS_DIFLAG_REALTIME and XFS_DIFLAG_EXTSIZE flags are
> mutually exclusive.

No, that's not true.

> XFS_DIFLAG_REALTIME and di_extsize have been introduced and used
> on Irix and Linux before XFS_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.

> 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.

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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: Review: factor extracting extent size hints from the inode

[Nathan Scott]

On Tue, 2007-06-12 at 16:08 +1000, David Chinner wrote:
> 
> 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. 

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.

-- 
Nathan

Re: Review: factor extracting extent size hints from the inode

[David Chinner]

On Tue, Jun 12, 2007 at 04:39:46PM +1000, Nathan Scott wrote:
> On Tue, 2007-06-12 at 16:08 +1000, David Chinner wrote:
> > 
> > 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. 
> 
> 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.....

> 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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group