Hi Lachlan,
I would prefer manual assignment here than struct assignment.
r[1].br_startoff and r[1].br_blockcount will be
modified immediately, so it is not worth assigning via ( r[1] = PREV) as
it does extra instructions.
Compiler would most likely eliminate the extra assignment but, why to
leave on the wit of the compiler.
It should be like
r[1].br_state = PREV.br_state;
r[1].br_startblock = 0 ; /* No fancy stuff required here as the aim here
is that br_startoff does not get any thing random */
Regards,
Shailendra
Lachlan McIlroy wrote:
This should be all that's needed. This code handles the case where
the middle
portion of a delayed allocation is being converted and splits the
extent into
three. The r[1] extent is the rightmost extent that will remain a
delayed
allocation. Both br_startblock and br_state need to be setup and they
will be
the same as the original delayed allocation (PREV) so we just inherit
those
values. Comments?
--- fs/xfs/xfs_bmap.c_1.358 2006-11-01 14:44:38.000000000 +0000
+++ fs/xfs/xfs_bmap.c 2006-11-02 13:22:41.000000000 +0000
@@ -1171,6 +1171,7 @@
xfs_bmap_trace_pre_update(fname, "0", ip, idx,
XFS_DATA_FORK);
xfs_bmbt_set_blockcount(ep, temp);
r[0] = *new;
+ r[1] = PREV;
r[1].br_startoff = new_endoff;
temp2 = PREV.br_startoff + PREV.br_blockcount -
new_endoff;
r[1].br_blockcount = temp2;
Lachlan
Vlad Apostolov wrote:
Hi Shailendra,
Shailendra Tripathi wrote:
Hi,
It appears that uninitialized r[3] in
xfs_bmap_add_extent_delay_real can potentially corrupt the startoff
for a particular case.
This sequence is below:
xfs_bmap_add_extent_delay_real (
...
xfs_bmbt_irec_t r[3]; /* neighbor extent entries */
case 0:
/*
* Filling in the middle part of a previous delayed
allocation.
* Contiguity is impossible here.
* This case is avoided almost all the time.
*/
temp = new->br_startoff - PREV.br_startoff;
xfs_bmbt_set_blockcount(ep, temp);
r[0] = *new;
r[1].br_startoff = new_endoff;
temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
r[1].br_blockcount = temp2;
xfs_bmap_insert_exlist(ip, idx + 1, 2, &r[0], XFS_DATA_FORK);
ip->i_df.if_lastex = idx + 1;
ip->i_d.di_nextents++;
Look at extent r[1]. It does not set br_startblock. That is, it is
any random value. Now, look at the xfs_bmbt_set_all. Though, it sets
the blockcount later, the startoff does not get changed.
#if XFS_BIG_BLKNOS
ASSERT((s->br_startblock & XFS_MASK64HI(12)) == 0);
r->l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
((xfs_bmbt_rec_base_t)s->br_startoff << 9) |
((xfs_bmbt_rec_base_t)s->br_startblock >> 43);
Top 21 bits are taken as it is. However, only 9 bit should be taken.
So, for random values, it corrupts the startoff which from 9-63 bits.
From the code inspection I agree with you that br_startblock doesn't
appear
to be initialized in this scenario. Otherwise I think the code looks
good.
If the br_startblock is initialized it should be a value that fits
in 52 bits out of 64 (this is what the ASSERT is for) and the top 12
bits will be 0.
The r->l0 gets the top 21 bits of br_startblock, the most significant
12 bits of
which are 0 and least significant 9 could be non 0. The r->l1 gets the
rest 43 (= 52-9 = 64-21) bits of br_startblock.
I will open a bug report for the uninitialized br_startblock.
Thank you for finding this problem.
Regards,
Vlad
r->l1 = ((xfs_bmbt_rec_base_t)s->br_startblock << 21) |
((xfs_bmbt_rec_base_t)s->br_blockcount &
(xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
I have attached a small program which does the same thing as it is
being done here. I would appreciate if someone can verify that
assertion is correct.
Regards,
Shailendra
------------------------------------------------------------------------
#include <stdio.h>
typedef unsigned long __uint64_t;
typedef struct xfs_bmbt_rec_64
{
__uint64_t l0, l1;
} xfs_bmbt_rec_64_t;
typedef __uint64_t xfs_bmbt_rec_base_t; typedef
xfs_bmbt_rec_64_t xfs_bmbt_rec_t, xfs_bmdr_rec_t;
typedef enum {
XFS_EXT_NORM, XFS_EXT_UNWRITTEN,
XFS_EXT_DMAPI_OFFLINE
} xfs_exntst_t;
typedef struct xfs_bmbt_irec
{
__uint64_t br_startoff; /* starting file offset */
__uint64_t br_startblock; /* starting block number */
__uint64_t br_blockcount; /* number of blocks */
xfs_exntst_t br_state; /* extent state */
} xfs_bmbt_irec_t;
#define XFS_MASK64LO(n) (((__uint64_t)1 << (n)) - 1)
#define XFS_MASK64HI(n) ((__uint64_t)-1 << (64 - (n)))
int main(void) {
xfs_bmbt_irec_t s;
xfs_bmbt_rec_t r;
int extent_flag;
s.br_startoff = 0;
s.br_blockcount = 5;
s.br_startblock = 0xfffffffffffffff0;
extent_flag = (s.br_state == XFS_EXT_NORM) ? 0 : 1;
printf("blockcount = 0x%llx\n", s.br_startblock);
r.l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
((xfs_bmbt_rec_base_t)s.br_startoff << 9) |
((xfs_bmbt_rec_base_t)s.br_startblock >> 43);
r.l1 = ((xfs_bmbt_rec_base_t)s.br_startblock << 21) |
((xfs_bmbt_rec_base_t)s.br_blockcount &
(xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
r.l0 = (r.l0 & (xfs_bmbt_rec_base_t)XFS_MASK64HI(55)) |
(xfs_bmbt_rec_base_t)((__uint64_t)100 >> 43);
r.l1 = (r.l1 & (xfs_bmbt_rec_base_t)XFS_MASK64LO(21)) |
(xfs_bmbt_rec_base_t)((__uint64_t)100 << 21);
printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
return 0;
}
|