2008-08-19 06:49:36

by Aneesh Kumar K.V

[permalink] [raw]
Subject: [PATCH] ext4: limit nrblocks properly with non extent format

We need to make sure adding new buffer_head to the
contiguous block extent doesn't result in nrblocks
greater than EXT4_MAX_TRANS_DATA. We only have
enought credit to insert EXT4_MAX_TRANS_DATA blocks.

Signed-off-by: Aneesh Kumar K.V <[email protected]>
---
fs/ext4/inode.c | 38 +++++++++++++++++++++++++-------------
1 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index dcd1337..a10f8e5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1909,27 +1909,39 @@ static int mpage_da_map_blocks(struct mpage_da_data *mpd)
static void mpage_add_bh_to_extent(struct mpage_da_data *mpd,
sector_t logical, struct buffer_head *bh)
{
- struct buffer_head *lbh = &mpd->lbh;
sector_t next;
+ size_t b_size = bh->b_size;
+ struct buffer_head *lbh = &mpd->lbh;
int nrblocks = lbh->b_size >> mpd->inode->i_blkbits;

/* check if thereserved journal credits might overflow */
- if (!(EXT4_I(mpd->inode)->i_flags & EXT4_EXTENTS_FL) &&
- (nrblocks >= EXT4_MAX_TRANS_DATA))
- /*
- * With noextent format we are limited by the journal
- * credit available. Total credit needed to insert
- * nrblocks contiguous blocks is dependent on the
- * nrblocks. So limit nrblocks.
- */
- goto flush_it;
-
+ if (!(EXT4_I(mpd->inode)->i_flags & EXT4_EXTENTS_FL)) {
+ if (nrblocks >= EXT4_MAX_TRANS_DATA) {
+ /*
+ * With noextent format we are limited by the journal
+ * credit available. Total credit needed to insert
+ * nrblocks contiguous blocks is dependent on the
+ * nrblocks. So limit nrblocks.
+ */
+ goto flush_it;
+ } else if ((nrblocks + (b_size >> mpd->inode->i_blkbits)) >
+ EXT4_MAX_TRANS_DATA) {
+ /*
+ * Adding the new buffer_head would make it cross the
+ * allowed limit for which we have journal credit
+ * reserved. So limit the new bh->b_size
+ */
+ b_size = (EXT4_MAX_TRANS_DATA - nrblocks) <<
+ mpd->inode->i_blkbits;
+ /* we will do mpage_da_submit_io in the next loop */
+ }
+ }
/*
* First block in the extent
*/
if (lbh->b_size == 0) {
lbh->b_blocknr = logical;
- lbh->b_size = bh->b_size;
+ lbh->b_size = b_size;
lbh->b_state = bh->b_state & BH_FLAGS;
return;
}
@@ -1939,7 +1951,7 @@ static void mpage_add_bh_to_extent(struct mpage_da_data *mpd,
* Can we merge the block to our big extent?
*/
if (logical == next && (bh->b_state & BH_FLAGS) == lbh->b_state) {
- lbh->b_size += bh->b_size;
+ lbh->b_size += b_size;
return;
}

--
1.6.0.2.g2ebc0



2008-08-19 06:48:24

by Aneesh Kumar K.V

[permalink] [raw]
Subject: [PATCH] ext4: Code cleanups

a) rename num to nrblocks to clearly indicate we expecting block count as argument.
b) renmae ext4_writepages_trans_blocks to ext4_da_writepages_trans_blocks to
indicate it is the delayed allocation writepages transaction credit.
c) Add some comments. remove some stale comments.

Signed-off-by: Aneesh Kumar K.V <[email protected]>
---
fs/ext4/extents.c | 8 ++++----
fs/ext4/inode.c | 14 ++++++++------
2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 73bb308..bf612a7 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1893,7 +1893,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
* When pass the actual path, the caller should calculate credits
* under i_data_sem.
*/
-int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int num,
+int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int nrblocks,
struct ext4_ext_path *path)
{
if (path) {
@@ -1912,12 +1912,12 @@ int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int num,
* and other metadat blocks still need to be
* accounted.
*/
- /* 1 one bitmap, 1 block group descriptor */
+ /* 1 bitmap, 1 block group descriptor */
ret = 2 + EXT4_META_TRANS_BLOCKS(inode->i_sb);
}
}

- return ext4_chunk_trans_blocks(inode, num);
+ return ext4_chunk_trans_blocks(inode, nrblocks);
}

/*
@@ -1931,7 +1931,7 @@ int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int num,
* If the nrblocks are discontiguous, they could cause
* the whole tree split more than once, but this is really rare.
*/
-int ext4_ext_index_trans_blocks(struct inode *inode, int num, int chunk)
+int ext4_ext_index_trans_blocks(struct inode *inode, int nrblocks, int chunk)
{
int index;
int depth = ext_depth(inode);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index a10f8e5..b32a723 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -2321,16 +2321,18 @@ static int ext4_da_writepage(struct page *page,
* a single extent allocation into a single transaction,
* ext4_da_writpeages() will loop calling this before
* the block allocation.
- *
- * The page vector size limited the max number of pages could
- * be writeout at a time. Based on this, the max blocks to pass to
- * get_block is calculated
*/

-static int ext4_writepages_trans_blocks(struct inode *inode)
+static int ext4_da_writepages_trans_blocks(struct inode *inode)
{
int max_blocks = EXT4_I(inode)->i_reserved_data_blocks;

+ /*
+ * With noextent format the journal credit needed to
+ * insert nrblocks contiguous block is dependent on
+ * number of contiguous block. So we will limit
+ * number of contiguous block to a sane value
+ */
if (!(inode->i_flags & EXT4_EXTENTS_FL) &&
(max_blocks > EXT4_MAX_TRANS_DATA))
max_blocks = EXT4_MAX_TRANS_DATA;
@@ -2387,7 +2389,7 @@ static int ext4_da_writepages(struct address_space *mapping,
* by delalloc
*/
BUG_ON(ext4_should_journal_data(inode));
- needed_blocks = ext4_writepages_trans_blocks(inode);
+ needed_blocks = ext4_da_writepages_trans_blocks(inode);

/* start a new transaction*/
handle = ext4_journal_start(inode, needed_blocks);
--
1.6.0.2.g2ebc0


2008-08-19 18:07:53

by Mingming Cao

[permalink] [raw]
Subject: Re: [PATCH] ext4: limit nrblocks properly with non extent format


在 2008-08-19二的 12:18 +0530,Aneesh Kumar K.V写道:
> We need to make sure adding new buffer_head to the
> contiguous block extent doesn't result in nrblocks
> greater than EXT4_MAX_TRANS_DATA. We only have
> enought credit to insert EXT4_MAX_TRANS_DATA blocks.
>
> Signed-off-by: Aneesh Kumar K.V <[email protected]>

Reviewed-by: Mingming Cao <[email protected]>
> ---
> fs/ext4/inode.c | 38 +++++++++++++++++++++++++-------------
> 1 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index dcd1337..a10f8e5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1909,27 +1909,39 @@ static int mpage_da_map_blocks(struct mpage_da_data *mpd)
> static void mpage_add_bh_to_extent(struct mpage_da_data *mpd,
> sector_t logical, struct buffer_head *bh)
> {
> - struct buffer_head *lbh = &mpd->lbh;
> sector_t next;
> + size_t b_size = bh->b_size;
> + struct buffer_head *lbh = &mpd->lbh;
> int nrblocks = lbh->b_size >> mpd->inode->i_blkbits;
>
> /* check if thereserved journal credits might overflow */
> - if (!(EXT4_I(mpd->inode)->i_flags & EXT4_EXTENTS_FL) &&
> - (nrblocks >= EXT4_MAX_TRANS_DATA))
> - /*
> - * With noextent format we are limited by the journal
> - * credit available. Total credit needed to insert
> - * nrblocks contiguous blocks is dependent on the
> - * nrblocks. So limit nrblocks.
> - */
> - goto flush_it;
> -
> + if (!(EXT4_I(mpd->inode)->i_flags & EXT4_EXTENTS_FL)) {
> + if (nrblocks >= EXT4_MAX_TRANS_DATA) {
> + /*
> + * With noextent format we are limited by the journal
> + * credit available. Total credit needed to insert
> + * nrblocks contiguous blocks is dependent on the
> + * nrblocks. So limit nrblocks.
> + */
> + goto flush_it;
> + } else if ((nrblocks + (b_size >> mpd->inode->i_blkbits)) >
> + EXT4_MAX_TRANS_DATA) {
> + /*
> + * Adding the new buffer_head would make it cross the
> + * allowed limit for which we have journal credit
> + * reserved. So limit the new bh->b_size
> + */
> + b_size = (EXT4_MAX_TRANS_DATA - nrblocks) <<
> + mpd->inode->i_blkbits;
> + /* we will do mpage_da_submit_io in the next loop */
> + }
> + }
> /*
> * First block in the extent
> */
> if (lbh->b_size == 0) {
> lbh->b_blocknr = logical;
> - lbh->b_size = bh->b_size;
> + lbh->b_size = b_size;
> lbh->b_state = bh->b_state & BH_FLAGS;
> return;
> }
> @@ -1939,7 +1951,7 @@ static void mpage_add_bh_to_extent(struct mpage_da_data *mpd,
> * Can we merge the block to our big extent?
> */
> if (logical == next && (bh->b_state & BH_FLAGS) == lbh->b_state) {
> - lbh->b_size += bh->b_size;
> + lbh->b_size += b_size;
> return;
> }
>

2008-08-19 18:29:37

by Mingming Cao

[permalink] [raw]
Subject: Re: [PATCH] ext4: Code cleanups


在 2008-08-19二的 12:18 +0530,Aneesh Kumar K.V写道:
> a) rename num to nrblocks to clearly indicate we expecting block count as argument.
> b) renmae ext4_writepages_trans_blocks to ext4_da_writepages_trans_blocks to
> indicate it is the delayed allocation writepages transaction credit.
> c) Add some comments. remove some stale comments.
>
> Signed-off-by: Aneesh Kumar K.V <[email protected]>

Acked-by: Mingming Cao <[email protected]>
> ---
> fs/ext4/extents.c | 8 ++++----
> fs/ext4/inode.c | 14 ++++++++------
> 2 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 73bb308..bf612a7 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1893,7 +1893,7 @@ static int ext4_ext_rm_idx(handle_t *handle, struct inode *inode,
> * When pass the actual path, the caller should calculate credits
> * under i_data_sem.
> */
> -int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int num,
> +int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int nrblocks,
> struct ext4_ext_path *path)
> {
> if (path) {
> @@ -1912,12 +1912,12 @@ int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int num,
> * and other metadat blocks still need to be
> * accounted.
> */
> - /* 1 one bitmap, 1 block group descriptor */
> + /* 1 bitmap, 1 block group descriptor */
> ret = 2 + EXT4_META_TRANS_BLOCKS(inode->i_sb);
> }
> }
>
> - return ext4_chunk_trans_blocks(inode, num);
> + return ext4_chunk_trans_blocks(inode, nrblocks);
> }
>
> /*
> @@ -1931,7 +1931,7 @@ int ext4_ext_calc_credits_for_single_extent(struct inode *inode, int num,
> * If the nrblocks are discontiguous, they could cause
> * the whole tree split more than once, but this is really rare.
> */
> -int ext4_ext_index_trans_blocks(struct inode *inode, int num, int chunk)
> +int ext4_ext_index_trans_blocks(struct inode *inode, int nrblocks, int chunk)
> {
> int index;
> int depth = ext_depth(inode);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index a10f8e5..b32a723 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2321,16 +2321,18 @@ static int ext4_da_writepage(struct page *page,
> * a single extent allocation into a single transaction,
> * ext4_da_writpeages() will loop calling this before
> * the block allocation.
> - *
> - * The page vector size limited the max number of pages could
> - * be writeout at a time. Based on this, the max blocks to pass to
> - * get_block is calculated
> */
>
> -static int ext4_writepages_trans_blocks(struct inode *inode)
> +static int ext4_da_writepages_trans_blocks(struct inode *inode)
> {
> int max_blocks = EXT4_I(inode)->i_reserved_data_blocks;
>
> + /*
> + * With noextent format the journal credit needed to
> + * insert nrblocks contiguous block is dependent on
> + * number of contiguous block. So we will limit
> + * number of contiguous block to a sane value
> + */
> if (!(inode->i_flags & EXT4_EXTENTS_FL) &&
> (max_blocks > EXT4_MAX_TRANS_DATA))
> max_blocks = EXT4_MAX_TRANS_DATA;
> @@ -2387,7 +2389,7 @@ static int ext4_da_writepages(struct address_space *mapping,
> * by delalloc
> */
> BUG_ON(ext4_should_journal_data(inode));
> - needed_blocks = ext4_writepages_trans_blocks(inode);
> + needed_blocks = ext4_da_writepages_trans_blocks(inode);
>
> /* start a new transaction*/
> handle = ext4_journal_start(inode, needed_blocks);