[02/11] ocfs2: free allocated clusters if error occurs after ocfs2_claim_clusters
diff mbox

Message ID 20140124204701.6AC005A4203@corp2gmr1-2.hot.corp.google.com
State New, archived
Headers show

Commit Message

Andrew Morton Jan. 24, 2014, 8:47 p.m. UTC
From: Zongxun Wang <wangzongxun@huawei.com>
Subject: ocfs2: free allocated clusters if error occurs after ocfs2_claim_clusters

Even if using the same jbd2 handle, we cannot rollback a transaction.  So
once some error occurs after successfully allocating clusters, the
allocated clusters will never be used and it means they are lost.  For
example, call ocfs2_claim_clusters successfully when expanding a file, but
failed in ocfs2_insert_extent.  So we need free the allocated clusters if
they are not used indeed.

Signed-off-by: Zongxun Wang <wangzongxun@huawei.com>
Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: Mark Fasheh <mfasheh@suse.com>
Cc: Li Zefan <lizefan@huawei.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/ocfs2/alloc.c      |   38 +++++++++++++++++++++++++++++++++++---
 fs/ocfs2/localalloc.c |   40 ++++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/localalloc.h |    6 ++++++
 3 files changed, 81 insertions(+), 3 deletions(-)

Comments

Joseph Qi Jan. 26, 2014, 2:53 a.m. UTC | #1
Hi

On 2014/1/25 4:47, akpm@linux-foundation.org wrote:
> From: Zongxun Wang <wangzongxun@huawei.com>
> Subject: ocfs2: free allocated clusters if error occurs after ocfs2_claim_clusters
> 
> Even if using the same jbd2 handle, we cannot rollback a transaction.  So
> once some error occurs after successfully allocating clusters, the
> allocated clusters will never be used and it means they are lost.  For
> example, call ocfs2_claim_clusters successfully when expanding a file, but
> failed in ocfs2_insert_extent.  So we need free the allocated clusters if
> they are not used indeed.
> 

We should note down num of bits to be freed, so as to update i_used
correspondingly after clearing those bits in bitmap.
I sent a patch based on this:
[PATCH] ocfs2: correctly update i_used in	ocfs2_free_local_alloc_bits
https://oss.oracle.com/pipermail/ocfs2-devel/2013-November/009462.html

> Signed-off-by: Zongxun Wang <wangzongxun@huawei.com>
> Signed-off-by: Joseph Qi <joseph.qi@huawei.com>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Mark Fasheh <mfasheh@suse.com>
> Cc: Li Zefan <lizefan@huawei.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/ocfs2/alloc.c      |   38 +++++++++++++++++++++++++++++++++++---
>  fs/ocfs2/localalloc.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  fs/ocfs2/localalloc.h |    6 ++++++
>  3 files changed, 81 insertions(+), 3 deletions(-)
> 
> diff -puN fs/ocfs2/alloc.c~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters fs/ocfs2/alloc.c
> --- a/fs/ocfs2/alloc.c~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters
> +++ a/fs/ocfs2/alloc.c
> @@ -4742,6 +4742,7 @@ int ocfs2_add_clusters_in_btree(handle_t
>  				enum ocfs2_alloc_restarted *reason_ret)
>  {
>  	int status = 0, err = 0;
> +	int need_free = 0;
>  	int free_extents;
>  	enum ocfs2_alloc_restarted reason = RESTART_NONE;
>  	u32 bit_off, num_bits;
> @@ -4796,7 +4797,8 @@ int ocfs2_add_clusters_in_btree(handle_t
>  					      OCFS2_JOURNAL_ACCESS_WRITE);
>  	if (status < 0) {
>  		mlog_errno(status);
> -		goto leave;
> +		need_free = 1;
> +		goto bail;
>  	}
>  
>  	block = ocfs2_clusters_to_blocks(osb->sb, bit_off);
> @@ -4807,7 +4809,8 @@ int ocfs2_add_clusters_in_btree(handle_t
>  				     num_bits, flags, meta_ac);
>  	if (status < 0) {
>  		mlog_errno(status);
> -		goto leave;
> +		need_free = 1;
> +		goto bail;
>  	}
>  
>  	ocfs2_journal_dirty(handle, et->et_root_bh);
> @@ -4821,6 +4824,19 @@ int ocfs2_add_clusters_in_btree(handle_t
>  		reason = RESTART_TRANS;
>  	}
>  
> +bail:
> +	if (need_free) {
> +		if (data_ac->ac_which == OCFS2_AC_USE_LOCAL)
> +			ocfs2_free_local_alloc_bits(osb, handle, data_ac,
> +					bit_off, num_bits);
> +		else
> +			ocfs2_free_clusters(handle,
> +					data_ac->ac_inode,
> +					data_ac->ac_bh,
> +					ocfs2_clusters_to_blocks(osb->sb, bit_off),
> +					num_bits);
> +	}
> +
>  leave:
>  	if (reason_ret)
>  		*reason_ret = reason;
> @@ -6805,6 +6821,8 @@ int ocfs2_convert_inline_data_to_extents
>  					 struct buffer_head *di_bh)
>  {
>  	int ret, i, has_data, num_pages = 0;
> +	int need_free = 0;
> +	u32 bit_off, num;
>  	handle_t *handle;
>  	u64 uninitialized_var(block);
>  	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> @@ -6850,7 +6868,6 @@ int ocfs2_convert_inline_data_to_extents
>  	}
>  
>  	if (has_data) {
> -		u32 bit_off, num;
>  		unsigned int page_end;
>  		u64 phys;
>  
> @@ -6886,6 +6903,7 @@ int ocfs2_convert_inline_data_to_extents
>  		ret = ocfs2_grab_eof_pages(inode, 0, end, pages, &num_pages);
>  		if (ret) {
>  			mlog_errno(ret);
> +			need_free = 1;
>  			goto out_commit;
>  		}
>  
> @@ -6896,6 +6914,7 @@ int ocfs2_convert_inline_data_to_extents
>  		ret = ocfs2_read_inline_data(inode, pages[0], di_bh);
>  		if (ret) {
>  			mlog_errno(ret);
> +			need_free = 1;
>  			goto out_commit;
>  		}
>  
> @@ -6927,6 +6946,7 @@ int ocfs2_convert_inline_data_to_extents
>  		ret = ocfs2_insert_extent(handle, &et, 0, block, 1, 0, NULL);
>  		if (ret) {
>  			mlog_errno(ret);
> +			need_free = 1;
>  			goto out_commit;
>  		}
>  
> @@ -6938,6 +6958,18 @@ out_commit:
>  		dquot_free_space_nodirty(inode,
>  					  ocfs2_clusters_to_bytes(osb->sb, 1));
>  
> +	if (need_free) {
> +		if (data_ac->ac_which == OCFS2_AC_USE_LOCAL)
> +			ocfs2_free_local_alloc_bits(osb, handle, data_ac,
> +					bit_off, num);
> +		else
> +			ocfs2_free_clusters(handle,
> +					data_ac->ac_inode,
> +					data_ac->ac_bh,
> +					ocfs2_clusters_to_blocks(osb->sb, bit_off),
> +					num);
> +	}
> +
>  	ocfs2_commit_trans(osb, handle);
>  
>  out_unlock:
> diff -puN fs/ocfs2/localalloc.c~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters fs/ocfs2/localalloc.c
> --- a/fs/ocfs2/localalloc.c~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters
> +++ a/fs/ocfs2/localalloc.c
> @@ -781,6 +781,46 @@ bail:
>  	return status;
>  }
>  
> +int ocfs2_free_local_alloc_bits(struct ocfs2_super *osb,
> +				handle_t *handle,
> +				struct ocfs2_alloc_context *ac,
> +				u32 bit_off,
> +				u32 num_bits)
> +{
> +	int status, start;
> +	struct inode *local_alloc_inode;
> +	void *bitmap;
> +	struct ocfs2_dinode *alloc;
> +	struct ocfs2_local_alloc *la;
> +
> +	BUG_ON(ac->ac_which != OCFS2_AC_USE_LOCAL);
> +
> +	local_alloc_inode = ac->ac_inode;
> +	alloc = (struct ocfs2_dinode *) osb->local_alloc_bh->b_data;
> +	la = OCFS2_LOCAL_ALLOC(alloc);
> +
> +	bitmap = la->la_bitmap;
> +	start = bit_off - le32_to_cpu(la->la_bm_off);
> +
> +	status = ocfs2_journal_access_di(handle,
> +			INODE_CACHE(local_alloc_inode),
> +			osb->local_alloc_bh,
> +			OCFS2_JOURNAL_ACCESS_WRITE);
> +	if (status < 0) {
> +		mlog_errno(status);
> +		goto bail;
> +	}
> +
> +	while (num_bits--)
> +		ocfs2_clear_bit(start++, bitmap);
> +
> +	le32_add_cpu(&alloc->id1.bitmap1.i_used, -num_bits);
> +	ocfs2_journal_dirty(handle, osb->local_alloc_bh);
> +
> +bail:
> +	return status;
> +}
> +
>  static u32 ocfs2_local_alloc_count_bits(struct ocfs2_dinode *alloc)
>  {
>  	u32 count;
> diff -puN fs/ocfs2/localalloc.h~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters fs/ocfs2/localalloc.h
> --- a/fs/ocfs2/localalloc.h~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters
> +++ a/fs/ocfs2/localalloc.h
> @@ -55,6 +55,12 @@ int ocfs2_claim_local_alloc_bits(struct
>  				 u32 *bit_off,
>  				 u32 *num_bits);
>  
> +int ocfs2_free_local_alloc_bits(struct ocfs2_super *osb,
> +				handle_t *handle,
> +				struct ocfs2_alloc_context *ac,
> +				u32 bit_off,
> +				u32 num_bits);
> +
>  void ocfs2_local_alloc_seen_free_bits(struct ocfs2_super *osb,
>  				      unsigned int num_clusters);
>  void ocfs2_la_enable_worker(struct work_struct *work);
> _
> 
> .
>
Andrew Morton Jan. 27, 2014, 11:07 p.m. UTC | #2
On Sun, 26 Jan 2014 10:53:24 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:

> Hi
> 
> On 2014/1/25 4:47, akpm@linux-foundation.org wrote:
> > From: Zongxun Wang <wangzongxun@huawei.com>
> > Subject: ocfs2: free allocated clusters if error occurs after ocfs2_claim_clusters
> > 
> > Even if using the same jbd2 handle, we cannot rollback a transaction.  So
> > once some error occurs after successfully allocating clusters, the
> > allocated clusters will never be used and it means they are lost.  For
> > example, call ocfs2_claim_clusters successfully when expanding a file, but
> > failed in ocfs2_insert_extent.  So we need free the allocated clusters if
> > they are not used indeed.
> > 
> 
> We should note down num of bits to be freed, so as to update i_used
> correspondingly after clearing those bits in bitmap.
> I sent a patch based on this:
> [PATCH] ocfs2: correctly update i_used in	ocfs2_free_local_alloc_bits
> https://oss.oracle.com/pipermail/ocfs2-devel/2013-November/009462.html

OK thanks, I now have that, as
ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters-fix.patch

Do we think that
ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters.patch
and
ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters-fix.patch
should now be merged upstream?
Joseph Qi Jan. 28, 2014, 1:02 a.m. UTC | #3
On 2014/1/28 7:07, Andrew Morton wrote:
> On Sun, 26 Jan 2014 10:53:24 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:
> 
>> Hi
>>
>> On 2014/1/25 4:47, akpm@linux-foundation.org wrote:
>>> From: Zongxun Wang <wangzongxun@huawei.com>
>>> Subject: ocfs2: free allocated clusters if error occurs after ocfs2_claim_clusters
>>>
>>> Even if using the same jbd2 handle, we cannot rollback a transaction.  So
>>> once some error occurs after successfully allocating clusters, the
>>> allocated clusters will never be used and it means they are lost.  For
>>> example, call ocfs2_claim_clusters successfully when expanding a file, but
>>> failed in ocfs2_insert_extent.  So we need free the allocated clusters if
>>> they are not used indeed.
>>>
>>
>> We should note down num of bits to be freed, so as to update i_used
>> correspondingly after clearing those bits in bitmap.
>> I sent a patch based on this:
>> [PATCH] ocfs2: correctly update i_used in	ocfs2_free_local_alloc_bits
>> https://oss.oracle.com/pipermail/ocfs2-devel/2013-November/009462.html
> 
> OK thanks, I now have that, as
> ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters-fix.patch
> 
> Do we think that
> ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters.patch
> and
> ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters-fix.patch
> should now be merged upstream?
> 
> 
> .
> 
Could Mark & Joel review the two patches? Thanks.
Joel Becker Jan. 31, 2014, 12:06 a.m. UTC | #4
On Tue, Jan 28, 2014 at 09:02:05AM +0800, Joseph Qi wrote:
> On 2014/1/28 7:07, Andrew Morton wrote:
> > On Sun, 26 Jan 2014 10:53:24 +0800 Joseph Qi <joseph.qi@huawei.com> wrote:
> > 
> >> Hi
> >>
> >> On 2014/1/25 4:47, akpm@linux-foundation.org wrote:
> >>> From: Zongxun Wang <wangzongxun@huawei.com>
> >>> Subject: ocfs2: free allocated clusters if error occurs after ocfs2_claim_clusters
> >>>
> >>> Even if using the same jbd2 handle, we cannot rollback a transaction.  So
> >>> once some error occurs after successfully allocating clusters, the
> >>> allocated clusters will never be used and it means they are lost.  For
> >>> example, call ocfs2_claim_clusters successfully when expanding a file, but
> >>> failed in ocfs2_insert_extent.  So we need free the allocated clusters if
> >>> they are not used indeed.
> >>>
> >>
> >> We should note down num of bits to be freed, so as to update i_used
> >> correspondingly after clearing those bits in bitmap.
> >> I sent a patch based on this:
> >> [PATCH] ocfs2: correctly update i_used in	ocfs2_free_local_alloc_bits
> >> https://oss.oracle.com/pipermail/ocfs2-devel/2013-November/009462.html
> > 
> > OK thanks, I now have that, as
> > ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters-fix.patch
> > 
> > Do we think that
> > ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters.patch
> > and
> > ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters-fix.patch
> > should now be merged upstream?

These patches combined look sane.  I'm curious what failed to cause this
to be noticed, but still:

Acked-by: Joel Becker <jlbec@evilplan.org>

> > 
> > 
> > .
> > 
> Could Mark & Joel review the two patches? Thanks.
>

Patch
diff mbox

diff -puN fs/ocfs2/alloc.c~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters fs/ocfs2/alloc.c
--- a/fs/ocfs2/alloc.c~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters
+++ a/fs/ocfs2/alloc.c
@@ -4742,6 +4742,7 @@  int ocfs2_add_clusters_in_btree(handle_t
 				enum ocfs2_alloc_restarted *reason_ret)
 {
 	int status = 0, err = 0;
+	int need_free = 0;
 	int free_extents;
 	enum ocfs2_alloc_restarted reason = RESTART_NONE;
 	u32 bit_off, num_bits;
@@ -4796,7 +4797,8 @@  int ocfs2_add_clusters_in_btree(handle_t
 					      OCFS2_JOURNAL_ACCESS_WRITE);
 	if (status < 0) {
 		mlog_errno(status);
-		goto leave;
+		need_free = 1;
+		goto bail;
 	}
 
 	block = ocfs2_clusters_to_blocks(osb->sb, bit_off);
@@ -4807,7 +4809,8 @@  int ocfs2_add_clusters_in_btree(handle_t
 				     num_bits, flags, meta_ac);
 	if (status < 0) {
 		mlog_errno(status);
-		goto leave;
+		need_free = 1;
+		goto bail;
 	}
 
 	ocfs2_journal_dirty(handle, et->et_root_bh);
@@ -4821,6 +4824,19 @@  int ocfs2_add_clusters_in_btree(handle_t
 		reason = RESTART_TRANS;
 	}
 
+bail:
+	if (need_free) {
+		if (data_ac->ac_which == OCFS2_AC_USE_LOCAL)
+			ocfs2_free_local_alloc_bits(osb, handle, data_ac,
+					bit_off, num_bits);
+		else
+			ocfs2_free_clusters(handle,
+					data_ac->ac_inode,
+					data_ac->ac_bh,
+					ocfs2_clusters_to_blocks(osb->sb, bit_off),
+					num_bits);
+	}
+
 leave:
 	if (reason_ret)
 		*reason_ret = reason;
@@ -6805,6 +6821,8 @@  int ocfs2_convert_inline_data_to_extents
 					 struct buffer_head *di_bh)
 {
 	int ret, i, has_data, num_pages = 0;
+	int need_free = 0;
+	u32 bit_off, num;
 	handle_t *handle;
 	u64 uninitialized_var(block);
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
@@ -6850,7 +6868,6 @@  int ocfs2_convert_inline_data_to_extents
 	}
 
 	if (has_data) {
-		u32 bit_off, num;
 		unsigned int page_end;
 		u64 phys;
 
@@ -6886,6 +6903,7 @@  int ocfs2_convert_inline_data_to_extents
 		ret = ocfs2_grab_eof_pages(inode, 0, end, pages, &num_pages);
 		if (ret) {
 			mlog_errno(ret);
+			need_free = 1;
 			goto out_commit;
 		}
 
@@ -6896,6 +6914,7 @@  int ocfs2_convert_inline_data_to_extents
 		ret = ocfs2_read_inline_data(inode, pages[0], di_bh);
 		if (ret) {
 			mlog_errno(ret);
+			need_free = 1;
 			goto out_commit;
 		}
 
@@ -6927,6 +6946,7 @@  int ocfs2_convert_inline_data_to_extents
 		ret = ocfs2_insert_extent(handle, &et, 0, block, 1, 0, NULL);
 		if (ret) {
 			mlog_errno(ret);
+			need_free = 1;
 			goto out_commit;
 		}
 
@@ -6938,6 +6958,18 @@  out_commit:
 		dquot_free_space_nodirty(inode,
 					  ocfs2_clusters_to_bytes(osb->sb, 1));
 
+	if (need_free) {
+		if (data_ac->ac_which == OCFS2_AC_USE_LOCAL)
+			ocfs2_free_local_alloc_bits(osb, handle, data_ac,
+					bit_off, num);
+		else
+			ocfs2_free_clusters(handle,
+					data_ac->ac_inode,
+					data_ac->ac_bh,
+					ocfs2_clusters_to_blocks(osb->sb, bit_off),
+					num);
+	}
+
 	ocfs2_commit_trans(osb, handle);
 
 out_unlock:
diff -puN fs/ocfs2/localalloc.c~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters fs/ocfs2/localalloc.c
--- a/fs/ocfs2/localalloc.c~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters
+++ a/fs/ocfs2/localalloc.c
@@ -781,6 +781,46 @@  bail:
 	return status;
 }
 
+int ocfs2_free_local_alloc_bits(struct ocfs2_super *osb,
+				handle_t *handle,
+				struct ocfs2_alloc_context *ac,
+				u32 bit_off,
+				u32 num_bits)
+{
+	int status, start;
+	struct inode *local_alloc_inode;
+	void *bitmap;
+	struct ocfs2_dinode *alloc;
+	struct ocfs2_local_alloc *la;
+
+	BUG_ON(ac->ac_which != OCFS2_AC_USE_LOCAL);
+
+	local_alloc_inode = ac->ac_inode;
+	alloc = (struct ocfs2_dinode *) osb->local_alloc_bh->b_data;
+	la = OCFS2_LOCAL_ALLOC(alloc);
+
+	bitmap = la->la_bitmap;
+	start = bit_off - le32_to_cpu(la->la_bm_off);
+
+	status = ocfs2_journal_access_di(handle,
+			INODE_CACHE(local_alloc_inode),
+			osb->local_alloc_bh,
+			OCFS2_JOURNAL_ACCESS_WRITE);
+	if (status < 0) {
+		mlog_errno(status);
+		goto bail;
+	}
+
+	while (num_bits--)
+		ocfs2_clear_bit(start++, bitmap);
+
+	le32_add_cpu(&alloc->id1.bitmap1.i_used, -num_bits);
+	ocfs2_journal_dirty(handle, osb->local_alloc_bh);
+
+bail:
+	return status;
+}
+
 static u32 ocfs2_local_alloc_count_bits(struct ocfs2_dinode *alloc)
 {
 	u32 count;
diff -puN fs/ocfs2/localalloc.h~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters fs/ocfs2/localalloc.h
--- a/fs/ocfs2/localalloc.h~ocfs2-free-allocated-clusters-if-error-occurs-after-ocfs2_claim_clusters
+++ a/fs/ocfs2/localalloc.h
@@ -55,6 +55,12 @@  int ocfs2_claim_local_alloc_bits(struct
 				 u32 *bit_off,
 				 u32 *num_bits);
 
+int ocfs2_free_local_alloc_bits(struct ocfs2_super *osb,
+				handle_t *handle,
+				struct ocfs2_alloc_context *ac,
+				u32 bit_off,
+				u32 num_bits);
+
 void ocfs2_local_alloc_seen_free_bits(struct ocfs2_super *osb,
 				      unsigned int num_clusters);
 void ocfs2_la_enable_worker(struct work_struct *work);