Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Work around GCC 4.8 aggressive loop optimization. #2010

Closed
wants to merge 1 commit into from

Conversation

dweeezil
Copy link
Contributor

GCC >+ 4.8's aggressive loop optimization breaks some of the iterators
over the dn_blkptr[] pseudo-array in dnode_phys. Since dn_blkptr[] is
defined as a single-element array, GCC believes an iterator can only
access index 0 and will unroll the loop into a single iteration.

This commit works around the optimization by casting the array to a
pointer and is definitely a hack. It fixes all obvious iterators that
might break, however, the only loop where it was known to cause problems
was this loop in dmu_objset_write_ready():

    for (i = 0; i < dnp->dn_nblkptr; i++)
            bp->blk_fill += dnp->dn_blkptr[i].blk_fill;

In the common case where dn_nblkptr is 3, the loop is only executed a
single time and "i" is equal to 1 following the loop.

The specific breakage cause by this problem is that the blk_fill of
root block pointers wouldn't be set properly when more than one blkptr
is in use (when no indrect blocks are needed).

The simple reproducing sequence is:

zpool create tank /tank.img
zdb -ddddd tank 0

and notice that "fill=31", however, there are two L0 indirect blocks with
"F=31" and "F=5". The fill count should be 36 rather than 35. This problem
causes an assert to be hit in a simple "zdb tank" when built with --enable-debug.

@dweeezil
Copy link
Contributor Author

I don't seriously expect my hack in this pull request to wind up as the production solution, however, it is something that works in the mean time for systems what use gcc 4.8. I suspect it's going to be a better idea to simply compile everything with -fno-aggressive-loop-optimizations. I'm mainly submitting this pull request to trigger an issue related to the problem and to present a workaround.

@behlendorf
Copy link
Contributor

@dweeezil As hacks go I've seen far worse. But what worries me is that in fact there are move cases in the code like this. We should just outright disable aggressive loop optimization if this is a problem. This can be done in a similar way to ZFS_AC_CONFIG_ALWAYS_NO_UNUSED_BUT_SET_VARIABLE and then added in to Rules.am. If you like, I can propose a patch to the build system for this.

@dweeezil
Copy link
Contributor Author

dweeezil commented Jan 7, 2014

@behlendorf If you could, please, that would be appreciated. I'm not terribly good with the autotools system (but I did look into it for this issue). We'd like to add -fno-aggressive-loop-optimizations to all builds when gcc >= 4.8 is in use.

So far, I've not found any other variable-sized arrays that will cause this problem. GCC Does The Right Thing when the variable array is the last member of the structure. That said, however, it's definitely a timebomb waiting to go off.

Matt Ahrens contacted me in a reply to my posting of this on the OpenZFS developers list that it might be a proper fix to convert the end of the dnode_phys_t to a union:

union {
  blkptr_t dn_blkptr[3];
  struct {
    blkptr_t dn_blkptr[1];
    uint8_t dn_bonus[DN_MAX_BONUSLEN];
  } dn_with_bonus;
  struct {
    blkptr_t dn_blkptr[1];
    uint8_t dn_bonus[DN_MAX_BONUSLEN - sizeof (blkptr_t)];
    blkptr_t dn_spill;
  } dn_with_spill;
} dn_u;

Of course, that would likely require changing a whole lot of code.

GCC >+ 4.8's aggressive loop optimization breaks some of the iterators
over the dn_blkptr[] pseudo-array in dnode_phys.  Since dn_blkptr[] is
defined as a single-element array, GCC believes an iterator can only
access index 0 and will unroll the loop into a single iteration.

This commit works around the optimization by casting the array to a
pointer and is definitely a hack.  It fixes all obvious iterators that
might break, however, the only loop where it was known to cause problems
was this loop in dmu_objset_write_ready():

        for (i = 0; i < dnp->dn_nblkptr; i++)
                bp->blk_fill += dnp->dn_blkptr[i].blk_fill;

In the common case where dn_nblkptr is 3, the loop is only executed a
single time and "i" is equal to 1 following the loop.

The specific breakage cause by this problem is that the blk_fill of
root block pointers wouldn't be set properly when more than one blkptr
is in use (when no indrect blocks are needed).

The simple reproducing sequence is:

	zpool create tank /tank.img
	zdb -ddddd tank 0

and notice that "fill=31", however, there are two L0 indirect blocks with
"F=31" and "F=5".  The fill count should be 36 rather than 35.  This problem
causes an assert to be hit in a simple "zdb tank" when built with --enable-debug.
behlendorf added a commit to behlendorf/zfs that referenced this pull request Jan 14, 2014
GCC >+ 4.8's aggressive loop optimization breaks some of the iterators
over the dn_blkptr[] pseudo-array in dnode_phys. Since dn_blkptr[] is
defined as a single-element array, GCC believes an iterator can only
access index 0 and will unroll the loop into a single iteration.

One way to resolve the issue would be to cast the array to a pointer
and fix all the iterators that might break.  The only loop where it
is known to cause a problem is this loop in dmu_objset_write_ready():

    for (i = 0; i < dnp->dn_nblkptr; i++)
            bp->blk_fill += dnp->dn_blkptr[i].blk_fill;

In the common case where dn_nblkptr is 3, the loop is only executed a
single time and "i" is equal to 1 following the loop.

The specific breakage caused by this problem is that the blk_fill of
root block pointers wouldn't be set properly when more than one blkptr
is in use (when no indrect blocks are needed).

The simple reproducing sequence is:

zpool create tank /tank.img
zdb -ddddd tank 0

Notice that "fill=31", however, there are two L0 indirect blocks with
"F=31" and "F=5". The fill count should be 36 rather than 35. This
problem causes an assert to be hit in a simple "zdb tank" when built
with --enable-debug.

However, this approach was not taken because we need to be absolutely
sure we catch all instances of this unwanted optimization.  Therefore,
the build system have been updated to detect if GCC supports the
aggressive loop optimization.  If it does the optimization will be
explicitly disabled using the -fno-aggressive loop-optimization option.

Original-fix-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#2010
@behlendorf
Copy link
Contributor

@dweeezil Can you please review #2051. It implements the build system changes to disable aggressive loop optimization we discussed. It still may be nice to rework the code as a union to avoid the issue but as you said that likely requires changing a whole lot of code.

@behlendorf behlendorf closed this Jan 14, 2014
behlendorf added a commit to behlendorf/zfs that referenced this pull request Jan 14, 2014
GCC >+ 4.8's aggressive loop optimization breaks some of the iterators
over the dn_blkptr[] pseudo-array in dnode_phys. Since dn_blkptr[] is
defined as a single-element array, GCC believes an iterator can only
access index 0 and will unroll the loop into a single iteration.

One way to resolve the issue would be to cast the array to a pointer
and fix all the iterators that might break.  The only loop where it
is known to cause a problem is this loop in dmu_objset_write_ready():

    for (i = 0; i < dnp->dn_nblkptr; i++)
            bp->blk_fill += dnp->dn_blkptr[i].blk_fill;

In the common case where dn_nblkptr is 3, the loop is only executed a
single time and "i" is equal to 1 following the loop.

The specific breakage caused by this problem is that the blk_fill of
root block pointers wouldn't be set properly when more than one blkptr
is in use (when no indrect blocks are needed).

The simple reproducing sequence is:

zpool create tank /tank.img
zdb -ddddd tank 0

Notice that "fill=31", however, there are two L0 indirect blocks with
"F=31" and "F=5". The fill count should be 36 rather than 31. This
problem causes an assert to be hit in a simple "zdb tank" when built
with --enable-debug.

However, this approach was not taken because we need to be absolutely
sure we catch all instances of this unwanted optimization.  Therefore,
the build system has been updated to detect if GCC supports the
aggressive loop optimization.  If it does the optimization will be
explicitly disabled using the -fno-aggressive-loop-optimization option.

Original-fix-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#2010
Closes openzfs#2051
ryao pushed a commit to ryao/zfs that referenced this pull request Apr 9, 2014
GCC >+ 4.8's aggressive loop optimization breaks some of the iterators
over the dn_blkptr[] pseudo-array in dnode_phys. Since dn_blkptr[] is
defined as a single-element array, GCC believes an iterator can only
access index 0 and will unroll the loop into a single iteration.

One way to resolve the issue would be to cast the array to a pointer
and fix all the iterators that might break.  The only loop where it
is known to cause a problem is this loop in dmu_objset_write_ready():

    for (i = 0; i < dnp->dn_nblkptr; i++)
            bp->blk_fill += dnp->dn_blkptr[i].blk_fill;

In the common case where dn_nblkptr is 3, the loop is only executed a
single time and "i" is equal to 1 following the loop.

The specific breakage caused by this problem is that the blk_fill of
root block pointers wouldn't be set properly when more than one blkptr
is in use (when no indrect blocks are needed).

The simple reproducing sequence is:

zpool create tank /tank.img
zdb -ddddd tank 0

Notice that "fill=31", however, there are two L0 indirect blocks with
"F=31" and "F=5". The fill count should be 36 rather than 31. This
problem causes an assert to be hit in a simple "zdb tank" when built
with --enable-debug.

However, this approach was not taken because we need to be absolutely
sure we catch all instances of this unwanted optimization.  Therefore,
the build system has been updated to detect if GCC supports the
aggressive loop optimization.  If it does the optimization will be
explicitly disabled using the -fno-aggressive-loop-optimization option.

Original-fix-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#2010
Closes openzfs#2051
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jul 18, 2014
This loop in dmu_objset_write_ready():

	for (i = 0; i < dnp->dn_nblkptr; i++)
		bp->blk_fill += dnp->dn_blkptr[i].blk_fill;

invokes _undefined behavior_ for the (common) case of dn_nblkptr=3,
therefore, the compiler is free to do whatever it wants (such as
optimizing it away, or otherwise messing up your expections).

The fix is to be honest about the array size.

Issue openzfs#2010
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jul 22, 2014
This loop in dmu_objset_write_ready():

	for (i = 0; i < dnp->dn_nblkptr; i++)
		bp->blk_fill += dnp->dn_blkptr[i].blk_fill;

invokes _undefined behavior_ for the (common) case of dn_nblkptr=3,
therefore, the compiler is free to do whatever it wants (such as
optimizing it away, or otherwise messing up your expections).

The fix is to be honest about the array size.

Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#2511
Closes openzfs#2010
behlendorf added a commit to behlendorf/zfs that referenced this pull request Jul 22, 2014
This reverts commit 0f62f3f.

Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#2010
ryao pushed a commit to ryao/zfs that referenced this pull request Nov 29, 2014
This reverts commit 0f62f3f.

Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#2010
@nedbass nedbass mentioned this pull request Jun 23, 2017
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants