* [PATCH] xfs_bmap_add_extent_delay_real should set br_startblock to nullstartblock
@ 2011-01-19 17:41 Ben Myers
2011-01-24 22:04 ` Dave Chinner
2011-01-27 18:52 ` Alex Elder
0 siblings, 2 replies; 5+ messages in thread
From: Ben Myers @ 2011-01-19 17:41 UTC (permalink / raw)
To: aelder; +Cc: xfs
When filling in the middle of a previous delayed allocation in
xfs_bmap_add_extent_delay_real, set br_startblock of the new delay extent to
the right to nullstartblock instead of 0 before inserting the extent into the
ifork (xfs_iext_insert), rather than setting br_startblock afterward.
Adding the extent into the ifork with br_startblock=0 can lead to the extent
being copied into the btree by xfs_bmap_extent_to_btree if we happen to convert
from extents format to btree format before updating br_startblock with the
correct value. The unexpected addition of this delay extent to the btree can
cause subsequent XFS_WANT_CORRUPTED_GOTO filesystem shutdown in several
xfs_bmap_add_extent_delay_real cases where we are converting a delay extent to
real and unexpectedly find an extent already inserted. For example:
911 case BMAP_LEFT_FILLING:
912 /*
913 * Filling in the first part of a previous delayed allocation.
914 * The left neighbor is not contiguous.
915 */
916 trace_xfs_bmap_pre_update(ip, idx, state, _THIS_IP_);
917 xfs_bmbt_set_startoff(ep, new_endoff);
918 temp = PREV.br_blockcount - new->br_blockcount;
919 xfs_bmbt_set_blockcount(ep, temp);
920 xfs_iext_insert(ip, idx, 1, new, state);
921 ip->i_df.if_lastex = idx;
922 ip->i_d.di_nextents++;
923 if (cur == NULL)
924 rval = XFS_ILOG_CORE | XFS_ILOG_DEXT;
925 else {
926 rval = XFS_ILOG_CORE;
927 if ((error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
928 new->br_startblock, new->br_blockcount,
929 &i)))
930 goto done;
931 XFS_WANT_CORRUPTED_GOTO(i == 0, done);
With the bogus extent in the btree we shutdown the filesystem at 931. The
conversion from extents to btree format happens when the number of extents in
the inode increases above ip->i_df.if_ext_max. xfs_bmap_extent_to_btree copies
extents from the ifork into the btree, ignoring all delalloc extents which are
denoted by br_startblock having some value of nullstartblock.
SGI-PV: 1013221
Signed-off-by: Ben Myers <bpm@sgi.com>
---
fs/xfs/xfs_bmap.c | 33 +++++++++++++++++++++++++--------
1 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 4111cd3..c1db779 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -1038,17 +1038,34 @@ xfs_bmap_add_extent_delay_real(
* Filling in the middle part of a previous delayed allocation.
* Contiguity is impossible here.
* This case is avoided almost all the time.
+ *
+ * We start with a delayed allocation:
+ *
+ * +ddddddddddddddddddddddddddddddddddddddddddddddddddddddd+
+ * PREV @ idx
+ *
+ * and we are allocating:
+ * +rrrrrrrrrrrrrrrrr+
+ * new
+ *
+ * and we set it up for insertion as:
+ * +ddddddddddddddddddd+rrrrrrrrrrrrrrrrr+ddddddddddddddddd+
+ * new
+ * PREV @ idx LEFT RIGHT
+ * inserted at idx + 1
*/
temp = new->br_startoff - PREV.br_startoff;
- trace_xfs_bmap_pre_update(ip, idx, 0, _THIS_IP_);
- xfs_bmbt_set_blockcount(ep, temp);
- r[0] = *new;
- r[1].br_state = PREV.br_state;
- r[1].br_startblock = 0;
- r[1].br_startoff = new_endoff;
temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
- r[1].br_blockcount = temp2;
- xfs_iext_insert(ip, idx + 1, 2, &r[0], state);
+ trace_xfs_bmap_pre_update(ip, idx, 0, _THIS_IP_);
+ xfs_bmbt_set_blockcount(ep, temp); /* truncate PREV */
+ LEFT = *new;
+ RIGHT.br_state = PREV.br_state;
+ RIGHT.br_startblock = nullstartblock(
+ (int)xfs_bmap_worst_indlen(ip, temp2));
+ RIGHT.br_startoff = new_endoff;
+ RIGHT.br_blockcount = temp2;
+ /* insert LEFT (r[0]) and RIGHT (r[1]) at the same time */
+ xfs_iext_insert(ip, idx + 1, 2, &LEFT, state);
ip->i_df.if_lastex = idx + 1;
ip->i_d.di_nextents++;
if (cur == NULL)
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] xfs_bmap_add_extent_delay_real should set br_startblock to nullstartblock
2011-01-19 17:41 [PATCH] xfs_bmap_add_extent_delay_real should set br_startblock to nullstartblock Ben Myers
@ 2011-01-24 22:04 ` Dave Chinner
2011-01-27 18:52 ` Alex Elder
1 sibling, 0 replies; 5+ messages in thread
From: Dave Chinner @ 2011-01-24 22:04 UTC (permalink / raw)
To: Ben Myers; +Cc: xfs, aelder
On Wed, Jan 19, 2011 at 11:41:58AM -0600, Ben Myers wrote:
> When filling in the middle of a previous delayed allocation in
> xfs_bmap_add_extent_delay_real, set br_startblock of the new delay extent to
> the right to nullstartblock instead of 0 before inserting the extent into the
> ifork (xfs_iext_insert), rather than setting br_startblock afterward.
>
> Adding the extent into the ifork with br_startblock=0 can lead to the extent
> being copied into the btree by xfs_bmap_extent_to_btree if we happen to convert
> from extents format to btree format before updating br_startblock with the
> correct value. The unexpected addition of this delay extent to the btree can
> cause subsequent XFS_WANT_CORRUPTED_GOTO filesystem shutdown in several
> xfs_bmap_add_extent_delay_real cases where we are converting a delay extent to
> real and unexpectedly find an extent already inserted. For example:
>
> 911 case BMAP_LEFT_FILLING:
> 912 /*
> 913 * Filling in the first part of a previous delayed allocation.
> 914 * The left neighbor is not contiguous.
> 915 */
> 916 trace_xfs_bmap_pre_update(ip, idx, state, _THIS_IP_);
> 917 xfs_bmbt_set_startoff(ep, new_endoff);
> 918 temp = PREV.br_blockcount - new->br_blockcount;
> 919 xfs_bmbt_set_blockcount(ep, temp);
> 920 xfs_iext_insert(ip, idx, 1, new, state);
> 921 ip->i_df.if_lastex = idx;
> 922 ip->i_d.di_nextents++;
> 923 if (cur == NULL)
> 924 rval = XFS_ILOG_CORE | XFS_ILOG_DEXT;
> 925 else {
> 926 rval = XFS_ILOG_CORE;
> 927 if ((error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
> 928 new->br_startblock, new->br_blockcount,
> 929 &i)))
> 930 goto done;
> 931 XFS_WANT_CORRUPTED_GOTO(i == 0, done);
>
> With the bogus extent in the btree we shutdown the filesystem at 931. The
> conversion from extents to btree format happens when the number of extents in
> the inode increases above ip->i_df.if_ext_max. xfs_bmap_extent_to_btree copies
> extents from the ifork into the btree, ignoring all delalloc extents which are
> denoted by br_startblock having some value of nullstartblock.
>
> SGI-PV: 1013221
>
> Signed-off-by: Ben Myers <bpm@sgi.com>
> ---
> fs/xfs/xfs_bmap.c | 33 +++++++++++++++++++++++++--------
> 1 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index 4111cd3..c1db779 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
> @@ -1038,17 +1038,34 @@ xfs_bmap_add_extent_delay_real(
> * Filling in the middle part of a previous delayed allocation.
> * Contiguity is impossible here.
> * This case is avoided almost all the time.
> + *
> + * We start with a delayed allocation:
> + *
> + * +ddddddddddddddddddddddddddddddddddddddddddddddddddddddd+
> + * PREV @ idx
> + *
> + * and we are allocating:
> + * +rrrrrrrrrrrrrrrrr+
> + * new
> + *
> + * and we set it up for insertion as:
> + * +ddddddddddddddddddd+rrrrrrrrrrrrrrrrr+ddddddddddddddddd+
> + * new
> + * PREV @ idx LEFT RIGHT
> + * inserted at idx + 1
> */
> temp = new->br_startoff - PREV.br_startoff;
> - trace_xfs_bmap_pre_update(ip, idx, 0, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> - r[0] = *new;
> - r[1].br_state = PREV.br_state;
> - r[1].br_startblock = 0;
> - r[1].br_startoff = new_endoff;
> temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
> - r[1].br_blockcount = temp2;
> - xfs_iext_insert(ip, idx + 1, 2, &r[0], state);
> + trace_xfs_bmap_pre_update(ip, idx, 0, _THIS_IP_);
> + xfs_bmbt_set_blockcount(ep, temp); /* truncate PREV */
> + LEFT = *new;
> + RIGHT.br_state = PREV.br_state;
> + RIGHT.br_startblock = nullstartblock(
> + (int)xfs_bmap_worst_indlen(ip, temp2));
> + RIGHT.br_startoff = new_endoff;
> + RIGHT.br_blockcount = temp2;
> + /* insert LEFT (r[0]) and RIGHT (r[1]) at the same time */
> + xfs_iext_insert(ip, idx + 1, 2, &LEFT, state);
> ip->i_df.if_lastex = idx + 1;
> ip->i_d.di_nextents++;
> if (cur == NULL)
Looks good.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xfs_bmap_add_extent_delay_real should set br_startblock to nullstartblock
2011-01-19 17:41 [PATCH] xfs_bmap_add_extent_delay_real should set br_startblock to nullstartblock Ben Myers
2011-01-24 22:04 ` Dave Chinner
@ 2011-01-27 18:52 ` Alex Elder
1 sibling, 0 replies; 5+ messages in thread
From: Alex Elder @ 2011-01-27 18:52 UTC (permalink / raw)
To: Ben Myers; +Cc: xfs
On Wed, 2011-01-19 at 11:41 -0600, Ben Myers wrote:
> When filling in the middle of a previous delayed allocation in
> xfs_bmap_add_extent_delay_real, set br_startblock of the new delay extent to
> the right to nullstartblock instead of 0 before inserting the extent into the
> ifork (xfs_iext_insert), rather than setting br_startblock afterward.
>
> Adding the extent into the ifork with br_startblock=0 can lead to the extent
> being copied into the btree by xfs_bmap_extent_to_btree if we happen to convert
> from extents format to btree format before updating br_startblock with the
> correct value. The unexpected addition of this delay extent to the btree can
> cause subsequent XFS_WANT_CORRUPTED_GOTO filesystem shutdown in several
> xfs_bmap_add_extent_delay_real cases where we are converting a delay extent to
> real and unexpectedly find an extent already inserted. For example:
. . .
> SGI-PV: 1013221
>
> Signed-off-by: Ben Myers <bpm@sgi.com>
> ---
> fs/xfs/xfs_bmap.c | 33 +++++++++++++++++++++++++--------
> 1 files changed, 25 insertions(+), 8 deletions(-)
This looks OK, but I want to point something out.
I'm not sure whether it's a significant problem
or not, but I think it could screw up some
reservation counts.
First though, it may be that the LEFT, RIGHT,
etc. were not used here because the xfs_bmbt_irec_t
array was not being used in the same way as it was
being used elsewhere in this function. Instead,
they are being overwritten with new extent data.
Same thing happens in xfs_bmap_add_extent_unwritten_real().
Nevertheless, I find that the symbolic names make
the result more readable, so I like the change.
Here's the thing I wanted to point out. This
change fixes it so the delalloc space after the
new extent has the correct NULL start block address
(encoding the worst case indirect block count in it).
Meanwhile, it does not update the NULL start block
address of PREV (and of course, this is how the
existing code works too). That means that the
indirect block count encoded therein may be
wrong as a result of the update.
I.e., in addition to:
xfs_bmbt_set_blockcount(ep, temp);
I think we ought to do:
int indlen;
indlen = (int) xfs_bmap_worst_indlen(ip, temp);
PREV.br_startblock = nullstartblock(indlen);
and then somehow update the in-core extent map with the
new information. (I don't know if there's an interface
to do that right now.)
Maybe the in-core map doesn't have to accurately reflect
the encoded indirect block reservation, I don't know.
If it does though, the code here (and likely elsewhere)
should probably be rearranged a bit so we can update all
three extents (PREV, LEFT (new), and RIGHT) with one call
to xfs_iext_update().
Note (again) that this doesn't say anything wrong
with this change, just that in looking at it I
noticed something else that might need to be fixed.
-Alex
> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index 4111cd3..c1db779 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
> @@ -1038,17 +1038,34 @@ xfs_bmap_add_extent_delay_real(
> * Filling in the middle part of a previous delayed allocation.
> * Contiguity is impossible here.
> * This case is avoided almost all the time.
> + *
> + * We start with a delayed allocation:
> + *
> + * +ddddddddddddddddddddddddddddddddddddddddddddddddddddddd+
> + * PREV @ idx
> + *
> + * and we are allocating:
> + * +rrrrrrrrrrrrrrrrr+
> + * new
> + *
> + * and we set it up for insertion as:
> + * +ddddddddddddddddddd+rrrrrrrrrrrrrrrrr+ddddddddddddddddd+
> + * new
> + * PREV @ idx LEFT RIGHT
> + * inserted at idx + 1
> */
> temp = new->br_startoff - PREV.br_startoff;
> - trace_xfs_bmap_pre_update(ip, idx, 0, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> - r[0] = *new;
> - r[1].br_state = PREV.br_state;
> - r[1].br_startblock = 0;
> - r[1].br_startoff = new_endoff;
> temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
> - r[1].br_blockcount = temp2;
> - xfs_iext_insert(ip, idx + 1, 2, &r[0], state);
Somehow I think this call, which inserts *two* extents, is
the reason this problem was a bit difficult to spot. Almost
all the other calls to this function insert just one.
> + trace_xfs_bmap_pre_update(ip, idx, 0, _THIS_IP_);
> + xfs_bmbt_set_blockcount(ep, temp); /* truncate PREV */
> + LEFT = *new;
> + RIGHT.br_state = PREV.br_state;
> + RIGHT.br_startblock = nullstartblock(
> + (int)xfs_bmap_worst_indlen(ip, temp2));
> + RIGHT.br_startoff = new_endoff;
> + RIGHT.br_blockcount = temp2;
> + /* insert LEFT (r[0]) and RIGHT (r[1]) at the same time */
> + xfs_iext_insert(ip, idx + 1, 2, &LEFT, state);
> ip->i_df.if_lastex = idx + 1;
> ip->i_d.di_nextents++;
> if (cur == NULL)
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] xfs_bmap_add_extent_delay_real should set br_startblock to nullstartblock
@ 2011-01-18 20:36 Ben Myers
2011-01-19 4:42 ` Dave Chinner
0 siblings, 1 reply; 5+ messages in thread
From: Ben Myers @ 2011-01-18 20:36 UTC (permalink / raw)
To: aelder; +Cc: xfs
When filling in the middle of a previous delayed allocation in
xfs_bmap_add_extent_delay_real, set br_startblock of the new delay extent to
the right to nullstartblock instead of 0 before inserting the extent into the
ifork (xfs_iext_insert), rather than setting br_startblock afterward.
Adding the extent into the ifork with br_startblock=0 can lead to the extent
being copied into the btree by xfs_bmap_extent_to_btree if we happen to convert
from extents format to btree format before updating br_startblock with the
correct value. The unexpected addition of this delay extent to the btree can
cause subsequent XFS_WANT_CORRUPTED_GOTO filesystem shutdown in several
xfs_bmap_add_extent_delay_real cases where we are converting a delay extent to
real and unexpectedly find an extent already inserted. For example:
911 case BMAP_LEFT_FILLING:
912 /*
913 * Filling in the first part of a previous delayed allocation.
914 * The left neighbor is not contiguous.
915 */
916 trace_xfs_bmap_pre_update(ip, idx, state, _THIS_IP_);
917 xfs_bmbt_set_startoff(ep, new_endoff);
918 temp = PREV.br_blockcount - new->br_blockcount;
919 xfs_bmbt_set_blockcount(ep, temp);
920 xfs_iext_insert(ip, idx, 1, new, state);
921 ip->i_df.if_lastex = idx;
922 ip->i_d.di_nextents++;
923 if (cur == NULL)
924 rval = XFS_ILOG_CORE | XFS_ILOG_DEXT;
925 else {
926 rval = XFS_ILOG_CORE;
927 if ((error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
928 new->br_startblock, new->br_blockcount,
929 &i)))
930 goto done;
931 XFS_WANT_CORRUPTED_GOTO(i == 0, done);
With the bogus extent in the btree we shutdown the filesystem at 931. The
conversion from extents to btree format happens when the number of extents in
the inode increases above ip->i_df.if_ext_max. xfs_bmap_extent_to_btree copies
extents from the ifork into the btree, ignoring all delalloc extents which are
denoted by br_startblock having some value of nullstartblock.
SGI-PV: 1013221
Signed-off-by: Ben Myers <bpm@sgi.com>
---
fs/xfs/xfs_bmap.c | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
index 4111cd3..754259f 100644
--- a/fs/xfs/xfs_bmap.c
+++ b/fs/xfs/xfs_bmap.c
@@ -1040,13 +1040,14 @@ xfs_bmap_add_extent_delay_real(
* This case is avoided almost all the time.
*/
temp = new->br_startoff - PREV.br_startoff;
+ temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
trace_xfs_bmap_pre_update(ip, idx, 0, _THIS_IP_);
- xfs_bmbt_set_blockcount(ep, temp);
- r[0] = *new;
- r[1].br_state = PREV.br_state;
- r[1].br_startblock = 0;
+ xfs_bmbt_set_blockcount(ep, temp); /* PREV becomes LEFT */
+ r[0] = *new; /* r[0] becomes MIDDLE */
+ r[1].br_state = PREV.br_state; /* r[1] is RIGHT */
+ r[1].br_startblock = nullstartblock(
+ (int)xfs_bmap_worst_indlen(ip, temp2));
r[1].br_startoff = new_endoff;
- temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
r[1].br_blockcount = temp2;
xfs_iext_insert(ip, idx + 1, 2, &r[0], state);
ip->i_df.if_lastex = idx + 1;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] xfs_bmap_add_extent_delay_real should set br_startblock to nullstartblock
2011-01-18 20:36 Ben Myers
@ 2011-01-19 4:42 ` Dave Chinner
0 siblings, 0 replies; 5+ messages in thread
From: Dave Chinner @ 2011-01-19 4:42 UTC (permalink / raw)
To: Ben Myers; +Cc: xfs, aelder
On Tue, Jan 18, 2011 at 02:36:29PM -0600, Ben Myers wrote:
> When filling in the middle of a previous delayed allocation in
> xfs_bmap_add_extent_delay_real, set br_startblock of the new delay extent to
> the right to nullstartblock instead of 0 before inserting the extent into the
> ifork (xfs_iext_insert), rather than setting br_startblock afterward.
>
> Adding the extent into the ifork with br_startblock=0 can lead to the extent
> being copied into the btree by xfs_bmap_extent_to_btree if we happen to convert
> from extents format to btree format before updating br_startblock with the
> correct value. The unexpected addition of this delay extent to the btree can
> cause subsequent XFS_WANT_CORRUPTED_GOTO filesystem shutdown in several
> xfs_bmap_add_extent_delay_real cases where we are converting a delay extent to
> real and unexpectedly find an extent already inserted. For example:
>
> 911 case BMAP_LEFT_FILLING:
> 912 /*
> 913 * Filling in the first part of a previous delayed allocation.
> 914 * The left neighbor is not contiguous.
> 915 */
> 916 trace_xfs_bmap_pre_update(ip, idx, state, _THIS_IP_);
> 917 xfs_bmbt_set_startoff(ep, new_endoff);
> 918 temp = PREV.br_blockcount - new->br_blockcount;
> 919 xfs_bmbt_set_blockcount(ep, temp);
> 920 xfs_iext_insert(ip, idx, 1, new, state);
> 921 ip->i_df.if_lastex = idx;
> 922 ip->i_d.di_nextents++;
> 923 if (cur == NULL)
> 924 rval = XFS_ILOG_CORE | XFS_ILOG_DEXT;
> 925 else {
> 926 rval = XFS_ILOG_CORE;
> 927 if ((error = xfs_bmbt_lookup_eq(cur, new->br_startoff,
> 928 new->br_startblock, new->br_blockcount,
> 929 &i)))
> 930 goto done;
> 931 XFS_WANT_CORRUPTED_GOTO(i == 0, done);
>
> With the bogus extent in the btree we shutdown the filesystem at 931. The
> conversion from extents to btree format happens when the number of extents in
> the inode increases above ip->i_df.if_ext_max. xfs_bmap_extent_to_btree copies
> extents from the ifork into the btree, ignoring all delalloc extents which are
> denoted by br_startblock having some value of nullstartblock.
>
> SGI-PV: 1013221
>
> Signed-off-by: Ben Myers <bpm@sgi.com>
> ---
> fs/xfs/xfs_bmap.c | 11 ++++++-----
> 1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> index 4111cd3..754259f 100644
> --- a/fs/xfs/xfs_bmap.c
> +++ b/fs/xfs/xfs_bmap.c
> @@ -1040,13 +1040,14 @@ xfs_bmap_add_extent_delay_real(
> * This case is avoided almost all the time.
> */
> temp = new->br_startoff - PREV.br_startoff;
> + temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
> trace_xfs_bmap_pre_update(ip, idx, 0, _THIS_IP_);
> - xfs_bmbt_set_blockcount(ep, temp);
> - r[0] = *new;
> - r[1].br_state = PREV.br_state;
> - r[1].br_startblock = 0;
> + xfs_bmbt_set_blockcount(ep, temp); /* PREV becomes LEFT */
> + r[0] = *new; /* r[0] becomes MIDDLE */
> + r[1].br_state = PREV.br_state; /* r[1] is RIGHT */
> + r[1].br_startblock = nullstartblock(
> + (int)xfs_bmap_worst_indlen(ip, temp2));
> r[1].br_startoff = new_endoff;
> - temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
> r[1].br_blockcount = temp2;
> xfs_iext_insert(ip, idx + 1, 2, &r[0], state);
> ip->i_df.if_lastex = idx + 1;
I'd still prefer that you replace r[?] with PREV/LEFT/RIGHT rather
than the array notation to be consistent with the rest of the code,
and add a comment something like this:
/*
* We start with:
*
* +ddddddddddddddddddddddddddddddddddddddddddddddddd+
* PREV @ idx
*
* and we are allocating:
* +rrrrrrrrrrrrrrr+
* new
*
* and we set it up for insertion as:
*
* +ddddddddddddddddddd+rrrrrrrrrrrrrrr+ddddddddddddd+
* new
* PREV @ idx LEFT RIGHT
* inserted @ idx + 1
*/
So that there is no ambiguity about the case being handled and how
it is executed....
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-01-27 18:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-19 17:41 [PATCH] xfs_bmap_add_extent_delay_real should set br_startblock to nullstartblock Ben Myers
2011-01-24 22:04 ` Dave Chinner
2011-01-27 18:52 ` Alex Elder
-- strict thread matches above, loose matches on Subject: below --
2011-01-18 20:36 Ben Myers
2011-01-19 4:42 ` Dave Chinner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox