[Ocfs2-devel] [PATCH 0/4] ocfs2: code cleanup for .36

[Ocfs2-devel] [PATCH 0/4] ocfs2: code cleanup for .36

[Tao Ma]

Hi Joel,
	These patches are generated when I read the codes. All are trivial 
cleanups. So I guess you can pull it easily, but I'd like to send them 
to the list first.

You can pull from
git://oss.oracle.com/git/tma/linux-2.6.git trivial

Regards,
Tao

[Ocfs2-devel] [PATCH 1/4] ocfs2/trivial: Move 'wanted' into parens of ocfs2_resmap_resv_bits.

[Tao Ma]

The first time I read the function ocfs2_resmap_resv_bits, I consider
about what 'wanted' will be used and consider about the comments.
Then I find it is only used if the reservation is empty. ;)

So we'd better move it to the parens so that it make the code more
readable, what's more, ocfs2_resmap_resv_bits is used so frequently
and we should save some cpus. The corresponding BUG_ON is also moved
into parens since it is only meaningful after we reinit the resv.

Cc: Mark Fasheh <mfasheh@suse.com>
Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/reservations.c |   26 ++++++++++++--------------
 1 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c
index d8b6e42..567c1a0 100644
--- a/fs/ocfs2/reservations.c
+++ b/fs/ocfs2/reservations.c
@@ -732,25 +732,23 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
 			   struct ocfs2_alloc_reservation *resv,
 			   int *cstart, int *clen)
 {
-	unsigned int wanted = *clen;
-
 	if (resv == NULL || ocfs2_resmap_disabled(resmap))
 		return -ENOSPC;
 
 	spin_lock(&resv_lock);
 
-	/*
-	 * We don't want to over-allocate for temporary
-	 * windows. Otherwise, we run the risk of fragmenting the
-	 * allocation space.
-	 */
-	wanted = ocfs2_resv_window_bits(resmap, resv);
-	if ((resv->r_flags & OCFS2_RESV_FLAG_TMP) || wanted < *clen)
-		wanted = *clen;
-
 	if (ocfs2_resv_empty(resv)) {
-		mlog(0, "empty reservation, find new window\n");
+		/*
+		 * We don't want to over-allocate for temporary
+		 * windows. Otherwise, we run the risk of fragmenting the
+		 * allocation space.
+		 */
+		unsigned int wanted = ocfs2_resv_window_bits(resmap, resv);
+
+		if ((resv->r_flags & OCFS2_RESV_FLAG_TMP) || wanted < *clen)
+			wanted = *clen;
 
+		mlog(0, "empty reservation, find new window\n");
 		/*
 		 * Try to get a window here. If it works, we must fall
 		 * through and test the bitmap . This avoids some
@@ -759,9 +757,9 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
 		 * that inode.
 		 */
 		ocfs2_resv_find_window(resmap, resv, wanted);
-	}
 
-	BUG_ON(ocfs2_resv_empty(resv));
+		BUG_ON(ocfs2_resv_empty(resv));
+	}
 
 	*cstart = resv->r_start;
 	*clen = resv->r_len;
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH 2/4] ocfs2: Remove unused old_id in ocfs2_commit_cache.

[Tao Ma]

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/journal.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 47878cf..2a9eac8 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -301,7 +301,6 @@ static int ocfs2_commit_cache(struct ocfs2_super *osb)
 {
 	int status = 0;
 	unsigned int flushed;
-	unsigned long old_id;
 	struct ocfs2_journal *journal = NULL;
 
 	mlog_entry_void();
@@ -326,7 +325,7 @@ static int ocfs2_commit_cache(struct ocfs2_super *osb)
 		goto finally;
 	}
 
-	old_id = ocfs2_inc_trans_id(journal);
+	ocfs2_inc_trans_id(journal);
 
 	flushed = atomic_read(&journal->j_num_trans);
 	atomic_set(&journal->j_num_trans, 0);
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH 3/4] ocfs2: Remove obsolete comments before ocfs2_start_trans.

[Tao Ma]

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/journal.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index 2a9eac8..ac2ea71 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -341,9 +341,6 @@ finally:
 	return status;
 }
 
-/* pass it NULL and it will allocate a new handle object for you.  If
- * you pass it a handle however, it may still return error, in which
- * case it has free'd the passed handle for you. */
 handle_t *ocfs2_start_trans(struct ocfs2_super *osb, int max_buffs)
 {
 	journal_t *journal = osb->journal->j_journal;
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH 4/4] ocfs2: Add some trace log for orphan scan.

[Tao Ma]

Now orphan scan worker has no trace log, so it is
very hard to tell whether it is finished or blocked.
So add 2 mlog trace log so that we can tell whether
the current orphan scan worker is blocked or not.
It does help when I analyzed an orphan scan bug.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
Signed-off-by: Sunil Mushran <sunil.mushran@oracle.com>
---
 fs/ocfs2/journal.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/journal.c b/fs/ocfs2/journal.c
index ac2ea71..9209f19 100644
--- a/fs/ocfs2/journal.c
+++ b/fs/ocfs2/journal.c
@@ -1887,6 +1887,8 @@ void ocfs2_queue_orphan_scan(struct ocfs2_super *osb)
 	if (atomic_read(&os->os_state) == ORPHAN_SCAN_INACTIVE)
 		goto out;
 
+	mlog(0, "Begin orphan scan\n");
+
 	status = ocfs2_orphan_scan_lock(osb, &seqno);
 	if (status < 0) {
 		if (status != -EAGAIN)
@@ -1915,6 +1917,7 @@ void ocfs2_queue_orphan_scan(struct ocfs2_super *osb)
 	os->os_scantime = CURRENT_TIME;
 unlock:
 	ocfs2_orphan_scan_unlock(osb, seqno);
+	mlog(0, "Orphan scan completed\n");
 out:
 	return;
 }
-- 
1.7.1.571.gba4d01

[Ocfs2-devel] [PATCH 2/4] ocfs2: Remove unused old_id in ocfs2_commit_cache.

[Mark Fasheh]

On Mon, Aug 02, 2010 at 11:02:13AM +0800, Tao Ma wrote:
> Signed-off-by: Tao Ma <tao.ma@oracle.com>
Acked-by: Mark Fasheh <mfasheh@suse.com>

--
Mark Fasheh

[Ocfs2-devel] [PATCH 3/4] ocfs2: Remove obsolete comments before ocfs2_start_trans.

[Mark Fasheh]

On Mon, Aug 02, 2010 at 11:02:14AM +0800, Tao Ma wrote:
> Signed-off-by: Tao Ma <tao.ma@oracle.com>
Acked-by: Mark Fasheh <mfasheh@suse.com>
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 2/4] ocfs2: Remove unused old_id in ocfs2_commit_cache.

[Joel Becker]

On Mon, Aug 02, 2010 at 11:02:13AM +0800, Tao Ma wrote:
> Signed-off-by: Tao Ma <tao.ma@oracle.com>

This patch is now in the merge-window branch of ocfs2.git.

Joel

-- 

"If you are ever in doubt as to whether or not to kiss a pretty girl, 
 give her the benefit of the doubt"
                                        -Thomas Carlyle

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH 3/4] ocfs2: Remove obsolete comments before ocfs2_start_trans.

[Joel Becker]

On Mon, Aug 02, 2010 at 11:02:14AM +0800, Tao Ma wrote:
> Signed-off-by: Tao Ma <tao.ma@oracle.com>

This patch is now in the merge-window branch of ocfs2.git.

Joel

-- 

Life's Little Instruction Book #237

	"Seek out the good in people."

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

[Ocfs2-devel] [PATCH 1/4] ocfs2/trivial: Move 'wanted' into parens of ocfs2_resmap_resv_bits.

[Mark Fasheh]

Hi Tao,

On Mon, Aug 02, 2010 at 11:02:12AM +0800, Tao Ma wrote:
> The first time I read the function ocfs2_resmap_resv_bits, I consider
> about what 'wanted' will be used and consider about the comments.
> Then I find it is only used if the reservation is empty. ;)
> 
> So we'd better move it to the parens so that it make the code more
> readable, what's more, ocfs2_resmap_resv_bits is used so frequently
> and we should save some cpus. The corresponding BUG_ON is also moved
> into parens since it is only meaningful after we reinit the resv.
> 
> Cc: Mark Fasheh <mfasheh@suse.com>
> Signed-off-by: Tao Ma <tao.ma@oracle.com>
> ---
>  fs/ocfs2/reservations.c |   26 ++++++++++++--------------
>  1 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c
> index d8b6e42..567c1a0 100644
> --- a/fs/ocfs2/reservations.c
> +++ b/fs/ocfs2/reservations.c
> @@ -732,25 +732,23 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
>  			   struct ocfs2_alloc_reservation *resv,
>  			   int *cstart, int *clen)
>  {
> -	unsigned int wanted = *clen;
> -
>  	if (resv == NULL || ocfs2_resmap_disabled(resmap))
>  		return -ENOSPC;
>  
>  	spin_lock(&resv_lock);
>  
> -	/*
> -	 * We don't want to over-allocate for temporary
> -	 * windows. Otherwise, we run the risk of fragmenting the
> -	 * allocation space.
> -	 */
> -	wanted = ocfs2_resv_window_bits(resmap, resv);
> -	if ((resv->r_flags & OCFS2_RESV_FLAG_TMP) || wanted < *clen)
> -		wanted = *clen;
> -
>  	if (ocfs2_resv_empty(resv)) {
> -		mlog(0, "empty reservation, find new window\n");
> +		/*
> +		 * We don't want to over-allocate for temporary
> +		 * windows. Otherwise, we run the risk of fragmenting the
> +		 * allocation space.
> +		 */
> +		unsigned int wanted = ocfs2_resv_window_bits(resmap, resv);
> +
> +		if ((resv->r_flags & OCFS2_RESV_FLAG_TMP) || wanted < *clen)
> +			wanted = *clen;
>  
> +		mlog(0, "empty reservation, find new window\n");
>  		/*
>  		 * Try to get a window here. If it works, we must fall
>  		 * through and test the bitmap . This avoids some
> @@ -759,9 +757,9 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
>  		 * that inode.
>  		 */
>  		ocfs2_resv_find_window(resmap, resv, wanted);
> -	}
>  
> -	BUG_ON(ocfs2_resv_empty(resv));
> +		BUG_ON(ocfs2_resv_empty(resv));
> +	}

Can we leave the BUG_ON() outside the if (ocfs2_resv_empty(...)) { } block?
You're technically correct in that the only possibility right now is if
there's a bug in ocfs2_resv_find_window(). However, I think it still belongs
outside the block because we're returning an allocation at that point. The
code is self documenting then - "don't let it get here unless resv->r_start
and resv->r_len mean something". That way any future changes to the function
will be forced to take this BUG_ON() into consideration, and we are then
less likely to accidentally corrupt data.

Otherwise the patch looks great to me. Thanks for the cleanup in the first
hunk!
	--Mark

--
Mark Fasheh

[Ocfs2-devel] [PATCH 1/4] ocfs2/trivial: Move 'wanted' into parens of ocfs2_resmap_resv_bits.

[Tao Ma]

Hi Mark,

On 08/14/2010 07:16 AM, Mark Fasheh wrote:
> Hi Tao,
>
> On Mon, Aug 02, 2010 at 11:02:12AM +0800, Tao Ma wrote:
>> The first time I read the function ocfs2_resmap_resv_bits, I consider
>> about what 'wanted' will be used and consider about the comments.
>> Then I find it is only used if the reservation is empty. ;)
>>
>> So we'd better move it to the parens so that it make the code more
>> readable, what's more, ocfs2_resmap_resv_bits is used so frequently
>> and we should save some cpus. The corresponding BUG_ON is also moved
>> into parens since it is only meaningful after we reinit the resv.
>>
>> Cc: Mark Fasheh<mfasheh@suse.com>
>> Signed-off-by: Tao Ma<tao.ma@oracle.com>
>> ---
>>   fs/ocfs2/reservations.c |   26 ++++++++++++--------------
>>   1 files changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c
>> index d8b6e42..567c1a0 100644
>> --- a/fs/ocfs2/reservations.c
>> +++ b/fs/ocfs2/reservations.c
>> @@ -732,25 +732,23 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
>>   			   struct ocfs2_alloc_reservation *resv,
>>   			   int *cstart, int *clen)
>>   {
>> -	unsigned int wanted = *clen;
>> -
>>   	if (resv == NULL || ocfs2_resmap_disabled(resmap))
>>   		return -ENOSPC;
>>
>>   	spin_lock(&resv_lock);
>>
>> -	/*
>> -	 * We don't want to over-allocate for temporary
>> -	 * windows. Otherwise, we run the risk of fragmenting the
>> -	 * allocation space.
>> -	 */
>> -	wanted = ocfs2_resv_window_bits(resmap, resv);
>> -	if ((resv->r_flags&  OCFS2_RESV_FLAG_TMP) || wanted<  *clen)
>> -		wanted = *clen;
>> -
>>   	if (ocfs2_resv_empty(resv)) {
>> -		mlog(0, "empty reservation, find new window\n");
>> +		/*
>> +		 * We don't want to over-allocate for temporary
>> +		 * windows. Otherwise, we run the risk of fragmenting the
>> +		 * allocation space.
>> +		 */
>> +		unsigned int wanted = ocfs2_resv_window_bits(resmap, resv);
>> +
>> +		if ((resv->r_flags&  OCFS2_RESV_FLAG_TMP) || wanted<  *clen)
>> +			wanted = *clen;
>>
>> +		mlog(0, "empty reservation, find new window\n");
>>   		/*
>>   		 * Try to get a window here. If it works, we must fall
>>   		 * through and test the bitmap . This avoids some
>> @@ -759,9 +757,9 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
>>   		 * that inode.
>>   		 */
>>   		ocfs2_resv_find_window(resmap, resv, wanted);
>> -	}
>>
>> -	BUG_ON(ocfs2_resv_empty(resv));
>> +		BUG_ON(ocfs2_resv_empty(resv));
>> +	}
>
> Can we leave the BUG_ON() outside the if (ocfs2_resv_empty(...)) { } block?
> You're technically correct in that the only possibility right now is if
> there's a bug in ocfs2_resv_find_window(). However, I think it still belongs
> outside the block because we're returning an allocation at that point. The
> code is self documenting then - "don't let it get here unless resv->r_start
> and resv->r_len mean something". That way any future changes to the function
> will be forced to take this BUG_ON() into consideration, and we are then
> less likely to accidentally corrupt data.
Actually I have thought of it. But please note that the if check is 
ocfs2_resv_empty. So does it look a little bit strange that:
if (ocfs2_resv_empty()) {
}

BUG_ON(ocfs2_resv_empty());

We have just checked ocfs2_resv_empty and now we BUG_ON it again?

Regards,
Tao