[Ocfs2-devel] [PATCH] Ocfs2: Rollback alloc_dinode counts when ocfs2_block_group_set_bits() failed
Younger Liu
younger.liucn at gmail.com
Mon Mar 31 17:37:40 PDT 2014
On 2014/4/1 5:02, Andrew Morton wrote:
> On Mon, 31 Mar 2014 21:07:40 +0800 Younger Liu <younger.liucn at gmail.com> wrote:
>
>> After updating alloc_dinode counts in ocfs2_alloc_dinode_update_counts(),
>> if ocfs2_alloc_dinode_update_bitmap() failed, there is a rare case that some
>> space may be lost.
>> So, we rollback alloc_dinode counts when ocfs2_block_group_set_bits() failed.
>
> This patch wildly conflicts with your earlier patch
> ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously.patch.
> below.
>
> What should be done?
I am sorry, about the earlier patch
ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously.patch.
I forget to tell you drop it from mm-tree.
Now, I follow the suggestion from Mark, and fix the same risk with a new approach:
Ocfs2-Rollback-dinode-counts-when-ocfs2_block_group_set_bits-failed.patch
Younger
>
>
> From: Younger Liu <younger.liucn at gmail.com>
> Subject: ocfs2: alloc_dinode counts and group bitmap should be update simultaneously
>
> Updating alloc_dinode counts in ocfs2_alloc_dinode_update_counts() and
> setting group bitmap in ocfs2_alloc_dinode_update_bitmap() have to be done
> simultaneously. Two cases are as follow:
>
> (1) If ocfs2_alloc_dinode_update_counts() fails, there is no need to
> set group bitmap. This case has been considered.
>
> (2) If ocfs2_alloc_dinode_update_bitmap() fails, alloc_dinode counts
> should be rolled back. Otherwise, some clusters would never be used.
> This case is not considered.
>
> So, we combine two functions, and ensure simultaneity.
>
> Signed-off-by: Younger Liu <younger.liucn at gmail.com>
> Cc: Joel Becker <jlbec at evilplan.org>
> Cc: Mark Fasheh <mfasheh at suse.com>
> Signed-off-by: Andrew Morton <akpm at linux-foundation.org>
> ---
>
> fs/ocfs2/move_extents.c | 11 --
> fs/ocfs2/ocfs2_trace.h | 2
> fs/ocfs2/suballoc.c | 155 +++++++++++++++-----------------------
> fs/ocfs2/suballoc.h | 21 +----
> 4 files changed, 77 insertions(+), 112 deletions(-)
>
> diff -puN fs/ocfs2/move_extents.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously fs/ocfs2/move_extents.c
> --- a/fs/ocfs2/move_extents.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
> +++ a/fs/ocfs2/move_extents.c
> @@ -681,18 +681,15 @@ static int ocfs2_move_extent(struct ocfs
> }
>
> gd = (struct ocfs2_group_desc *)gd_bh->b_data;
> - ret = ocfs2_alloc_dinode_update_counts(gb_inode, handle, gb_bh, len,
> - le16_to_cpu(gd->bg_chain));
> + ret = ocfs2_alloc_dinode_update_bitmap(handle,
> + gb_inode, gb_bh, gd, gb_bh,
> + le16_to_cpu(gd->bg_chain),
> + goal_bit, len);
> if (ret) {
> mlog_errno(ret);
> goto out_commit;
> }
>
> - ret = ocfs2_block_group_set_bits(handle, gb_inode, gd, gd_bh,
> - goal_bit, len);
> - if (ret)
> - mlog_errno(ret);
> -
> /*
> * Here we should write the new page out first if we are
> * in write-back mode.
> diff -puN fs/ocfs2/ocfs2_trace.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously fs/ocfs2/ocfs2_trace.h
> --- a/fs/ocfs2/ocfs2_trace.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
> +++ a/fs/ocfs2/ocfs2_trace.h
> @@ -788,7 +788,7 @@ DEFINE_OCFS2_UINT_UINT_UINT_EVENT(ocfs2_
>
> DEFINE_OCFS2_ULL_EVENT(ocfs2_reserve_new_inode_new_group);
>
> -DEFINE_OCFS2_UINT_UINT_EVENT(ocfs2_block_group_set_bits);
> +DEFINE_OCFS2_UINT_UINT_EVENT(ocfs2_alloc_dinode_update_bitmap);
>
> TRACE_EVENT(ocfs2_relink_block_group,
> TP_PROTO(unsigned long long i_blkno, unsigned int chain,
> diff -puN fs/ocfs2/suballoc.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously fs/ocfs2/suballoc.c
> --- a/fs/ocfs2/suballoc.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
> +++ a/fs/ocfs2/suballoc.c
> @@ -1337,54 +1337,6 @@ static int ocfs2_block_group_find_clear_
> return status;
> }
>
> -int ocfs2_block_group_set_bits(handle_t *handle,
> - struct inode *alloc_inode,
> - struct ocfs2_group_desc *bg,
> - struct buffer_head *group_bh,
> - unsigned int bit_off,
> - unsigned int num_bits)
> -{
> - int status;
> - void *bitmap = bg->bg_bitmap;
> - int journal_type = OCFS2_JOURNAL_ACCESS_WRITE;
> -
> - /* All callers get the descriptor via
> - * ocfs2_read_group_descriptor(). Any corruption is a code bug. */
> - BUG_ON(!OCFS2_IS_VALID_GROUP_DESC(bg));
> - BUG_ON(le16_to_cpu(bg->bg_free_bits_count) < num_bits);
> -
> - trace_ocfs2_block_group_set_bits(bit_off, num_bits);
> -
> - if (ocfs2_is_cluster_bitmap(alloc_inode))
> - journal_type = OCFS2_JOURNAL_ACCESS_UNDO;
> -
> - status = ocfs2_journal_access_gd(handle,
> - INODE_CACHE(alloc_inode),
> - group_bh,
> - journal_type);
> - if (status < 0) {
> - mlog_errno(status);
> - goto bail;
> - }
> -
> - le16_add_cpu(&bg->bg_free_bits_count, -num_bits);
> - if (le16_to_cpu(bg->bg_free_bits_count) > le16_to_cpu(bg->bg_bits)) {
> - ocfs2_error(alloc_inode->i_sb, "Group descriptor # %llu has bit"
> - " count %u but claims %u are freed. num_bits %d",
> - (unsigned long long)le64_to_cpu(bg->bg_blkno),
> - le16_to_cpu(bg->bg_bits),
> - le16_to_cpu(bg->bg_free_bits_count), num_bits);
> - return -EROFS;
> - }
> - while(num_bits--)
> - ocfs2_set_bit(bit_off++, bitmap);
> -
> - ocfs2_journal_dirty(handle, group_bh);
> -
> -bail:
> - return status;
> -}
> -
> /* find the one with the most empty bits */
> static inline u16 ocfs2_find_victim_chain(struct ocfs2_chain_list *cl)
> {
> @@ -1580,31 +1532,78 @@ static int ocfs2_block_group_search(stru
> return ret;
> }
>
> -int ocfs2_alloc_dinode_update_counts(struct inode *inode,
> - handle_t *handle,
> - struct buffer_head *di_bh,
> - u32 num_bits,
> - u16 chain)
> +int ocfs2_alloc_dinode_update_bitmap(handle_t *handle,
> + struct inode *alloc_inode,
> + struct buffer_head *di_bh,
> + struct ocfs2_group_desc *bg,
> + struct buffer_head *group_bh,
> + u16 chain, u32 bit_off, u32 num_bits)
> {
> int ret;
> u32 tmp_used;
> struct ocfs2_dinode *di = (struct ocfs2_dinode *) di_bh->b_data;
> struct ocfs2_chain_list *cl = (struct ocfs2_chain_list *) &di->id2.i_chain;
> + void *bitmap = bg->bg_bitmap;
> + int journal_type = OCFS2_JOURNAL_ACCESS_WRITE;
>
> - ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
> - OCFS2_JOURNAL_ACCESS_WRITE);
> + /*
> + * All callers get the descriptor via
> + * ocfs2_read_group_descriptor(). Any corruption is a code bug.
> + */
> + BUG_ON(!OCFS2_IS_VALID_GROUP_DESC(bg));
> + BUG_ON(le16_to_cpu(bg->bg_free_bits_count) < num_bits);
> +
> + trace_ocfs2_alloc_dinode_update_bitmap(bit_off, num_bits);
> +
> + ret = ocfs2_journal_access_di(handle,
> + INODE_CACHE(alloc_inode), di_bh, journal_type);
> if (ret < 0) {
> mlog_errno(ret);
> goto out;
> }
>
> + if (ocfs2_is_cluster_bitmap(alloc_inode))
> + journal_type = OCFS2_JOURNAL_ACCESS_UNDO;
> +
> + ret = ocfs2_journal_access_gd(handle,
> + INODE_CACHE(alloc_inode), group_bh, journal_type);
> + if (ret < 0) {
> + mlog_errno(ret);
> + goto out;
> + }
> +
> + /* update alloc_dinode counts */
> tmp_used = le32_to_cpu(di->id1.bitmap1.i_used);
> di->id1.bitmap1.i_used = cpu_to_le32(num_bits + tmp_used);
> le32_add_cpu(&cl->cl_recs[chain].c_free, -num_bits);
> +
> + /* update bg counts and bitmap*/
> + le16_add_cpu(&bg->bg_free_bits_count, -num_bits);
> + if (le16_to_cpu(bg->bg_free_bits_count) > le16_to_cpu(bg->bg_bits)) {
> + ocfs2_error(alloc_inode->i_sb, "Group descriptor # %llu has bit"
> + " count %u but claims %u are freed. num_bits %d",
> + (unsigned long long)le64_to_cpu(bg->bg_blkno),
> + le16_to_cpu(bg->bg_bits),
> + le16_to_cpu(bg->bg_free_bits_count), num_bits);
> + ret = -EROFS;
> + goto out_rollback;
> + }
> + while (num_bits--)
> + ocfs2_set_bit(bit_off++, bitmap);
> +
> ocfs2_journal_dirty(handle, di_bh);
> + ocfs2_journal_dirty(handle, group_bh);
>
> out:
> return ret;
> +
> +out_rollback:
> + le16_add_cpu(&bg->bg_free_bits_count, num_bits);
> +
> + di->id1.bitmap1.i_used = cpu_to_le32(tmp_used - num_bits);
> + le32_add_cpu(&cl->cl_recs[chain].c_free, num_bits);
> +
> + return ret;
> }
>
> static int ocfs2_bg_discontig_fix_by_rec(struct ocfs2_suballoc_result *res,
> @@ -1697,19 +1696,15 @@ static int ocfs2_search_one_group(struct
> if (ac->ac_find_loc_only)
> goto out_loc_only;
>
> - ret = ocfs2_alloc_dinode_update_counts(alloc_inode, handle, ac->ac_bh,
> - res->sr_bits,
> - le16_to_cpu(gd->bg_chain));
> + ret = ocfs2_alloc_dinode_update_bitmap(handle,
> + alloc_inode, ac->ac_bh, gd, group_bh,
> + le16_to_cpu(gd->bg_chain),
> + res->sr_bit_offset, res->sr_bits);
> if (ret < 0) {
> mlog_errno(ret);
> goto out;
> }
>
> - ret = ocfs2_block_group_set_bits(handle, alloc_inode, gd, group_bh,
> - res->sr_bit_offset, res->sr_bits);
> - if (ret < 0)
> - mlog_errno(ret);
> -
> out_loc_only:
> *bits_left = le16_to_cpu(gd->bg_free_bits_count);
>
> @@ -1823,20 +1818,9 @@ static int ocfs2_search_chain(struct ocf
> if (ac->ac_find_loc_only)
> goto out_loc_only;
>
> - status = ocfs2_alloc_dinode_update_counts(alloc_inode, handle,
> - ac->ac_bh, res->sr_bits,
> - chain);
> - if (status) {
> - mlog_errno(status);
> - goto bail;
> - }
> -
> - status = ocfs2_block_group_set_bits(handle,
> - alloc_inode,
> - bg,
> - group_bh,
> - res->sr_bit_offset,
> - res->sr_bits);
> + status = ocfs2_alloc_dinode_update_bitmap(handle,
> + alloc_inode, ac->ac_bh, bg, group_bh,
> + chain, res->sr_bit_offset, res->sr_bits);
> if (status < 0) {
> mlog_errno(status);
> goto bail;
> @@ -2134,20 +2118,9 @@ int ocfs2_claim_new_inode_at_loc(handle_
> bg = (struct ocfs2_group_desc *) bg_bh->b_data;
> chain = le16_to_cpu(bg->bg_chain);
>
> - ret = ocfs2_alloc_dinode_update_counts(ac->ac_inode, handle,
> - ac->ac_bh, res->sr_bits,
> - chain);
> - if (ret) {
> - mlog_errno(ret);
> - goto out;
> - }
> -
> - ret = ocfs2_block_group_set_bits(handle,
> - ac->ac_inode,
> - bg,
> - bg_bh,
> - res->sr_bit_offset,
> - res->sr_bits);
> + ret = ocfs2_alloc_dinode_update_bitmap(handle,
> + ac->ac_inode, ac->ac_bh, bg, bg_bh,
> + chain, res->sr_bit_offset, res->sr_bits);
> if (ret < 0) {
> mlog_errno(ret);
> goto out;
> diff -puN fs/ocfs2/suballoc.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously fs/ocfs2/suballoc.h
> --- a/fs/ocfs2/suballoc.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously
> +++ a/fs/ocfs2/suballoc.h
> @@ -85,19 +85,14 @@ int ocfs2_reserve_new_inode(struct ocfs2
> int ocfs2_reserve_clusters(struct ocfs2_super *osb,
> u32 bits_wanted,
> struct ocfs2_alloc_context **ac);
> -
> -int ocfs2_alloc_dinode_update_counts(struct inode *inode,
> - handle_t *handle,
> - struct buffer_head *di_bh,
> - u32 num_bits,
> - u16 chain);
> -int ocfs2_block_group_set_bits(handle_t *handle,
> - struct inode *alloc_inode,
> - struct ocfs2_group_desc *bg,
> - struct buffer_head *group_bh,
> - unsigned int bit_off,
> - unsigned int num_bits);
> -
> +int ocfs2_alloc_dinode_update_bitmap(handle_t *handle,
> + struct inode *alloc_inode,
> + struct buffer_head *di_bh,
> + struct ocfs2_group_desc *bg,
> + struct buffer_head *group_bh,
> + u16 chain,
> + u32 bit_off,
> + u32 num_bits);
> int ocfs2_claim_metadata(handle_t *handle,
> struct ocfs2_alloc_context *ac,
> u32 bits_wanted,
> _
>
>
>
More information about the Ocfs2-devel
mailing list