[PATCH 1/2] Staging: lustre: Fix line length exceeding 80 characters

[PATCH 1/2] Staging: lustre: Fix line length exceeding 80 characters

[MonamAgarwal]

This patch fixes the following checkpatch.pl warning in
lustre/ldlm/interval_tree.c
WARNING: line over 80 characters in the file

Signed-off-by: MonamAgarwal <monamagarwal123@gmail.com>
---
 drivers/staging/lustre/lustre/ldlm/interval_tree.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/interval_tree.c b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
index 1de1d8e..f61bf19 100644
--- a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
+++ b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
@@ -429,7 +429,8 @@ static void interval_erase_color(struct interval_node *node,
 					struct interval_node *o_left;
 					o_left = tmp->in_left;
 					if (o_left)
-						o_left->in_color = INTERVAL_BLACK;
+						o_left->in_color =
+							INTERVAL_BLACK;
 					tmp->in_color = INTERVAL_RED;
 					__rotate_right(tmp, root);
 					tmp = parent->in_right;
@@ -437,7 +438,8 @@ static void interval_erase_color(struct interval_node *node,
 				tmp->in_color = parent->in_color;
 				parent->in_color = INTERVAL_BLACK;
 				if (tmp->in_right)
-					tmp->in_right->in_color = INTERVAL_BLACK;
+					tmp->in_right->in_color =
+						INTERVAL_BLACK;
 				__rotate_left(parent, root);
 				node = *root;
 				break;
@@ -460,7 +462,8 @@ static void interval_erase_color(struct interval_node *node,
 					struct interval_node *o_right;
 					o_right = tmp->in_right;
 					if (o_right)
-						o_right->in_color = INTERVAL_BLACK;
+						o_right->in_color =
+							INTERVAL_BLACK;
 					tmp->in_color = INTERVAL_RED;
 					__rotate_left(tmp, root);
-- 					tmp = parent->in_left;
1.7.9.5

Re: [PATCH 1/2] Staging: lustre: Fix line length exceeding 80 characters

[Joe Perches]

On Mon, 2014-01-06 at 01:41 +0530, MonamAgarwal wrote:
> This patch fixes the following checkpatch.pl warning in
> lustre/ldlm/interval_tree.c
> WARNING: line over 80 characters in the file
[]
> diff --git a/drivers/staging/lustre/lustre/ldlm/interval_tree.c b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
[]
> @@ -429,7 +429,8 @@ static void interval_erase_color(struct interval_node *node,
>  					struct interval_node *o_left;
>  					o_left = tmp->in_left;
>  					if (o_left)
> -						o_left->in_color = INTERVAL_BLACK;
> +						o_left->in_color =
> +							INTERVAL_BLACK;

Likely this function would be better off with some
refactoring instead of straining to fit 80 cols.

Re: [PATCH 1/2] Staging: lustre: Fix line length exceeding 80 characters

[Greg KH]

On Sun, Jan 05, 2014 at 12:30:51PM -0800, Joe Perches wrote:
> On Mon, 2014-01-06 at 01:41 +0530, MonamAgarwal wrote:
> > This patch fixes the following checkpatch.pl warning in
> > lustre/ldlm/interval_tree.c
> > WARNING: line over 80 characters in the file
> []
> > diff --git a/drivers/staging/lustre/lustre/ldlm/interval_tree.c b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> []
> > @@ -429,7 +429,8 @@ static void interval_erase_color(struct interval_node *node,
> >  					struct interval_node *o_left;
> >  					o_left = tmp->in_left;
> >  					if (o_left)
> > -						o_left->in_color = INTERVAL_BLACK;
> > +						o_left->in_color =
> > +							INTERVAL_BLACK;
> 
> Likely this function would be better off with some
> refactoring instead of straining to fit 80 cols.

I agree, if you are doing things like this, that's a huge hint that the
code needs fixing.

thanks,

greg k-h

[PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Monam Agarwal]

The function interval_erase_color() in lustre/ldlm/interval_tree.c is
too long and can be refactored. Most of the statements are same for if
and else conditions. I am passing a new variable n based on which the
differences are recognised.

This fixes the following checkpatch.pl warning in
lustre/ldlm/interval_tree.c
WARNING: line over 80 characters in the file

Signed-off-by: Monam Agarwal <monamagarwal123@gmail.com>
---

Changes since version 1:
* Incorrect fixing
* Incorrect Signed-off-by line

Changes since version 2:
* Removed new line character

 drivers/staging/lustre/lustre/ldlm/interval_tree.c |  128 +++++++++++---------
 1 file changed, 69 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/interval_tree.c b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
index 1de1d8e..7c956de 100644
--- a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
+++ b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
@@ -404,75 +404,85 @@ static inline int node_is_black_or_0(struct interval_node *node)
 	return !node || node_is_black(node);
 }
 
+static int interval_set_color(struct interval_node *node,
+			      struct interval_node *parent,
+			      struct interval_node *root,
+			      int n)
+{
+	struct interval_node *tmp;
+	if (n == 0)
+		tmp = parent->in_right;
+	else
+		tmp = parent->in_left;
+	if (node_is_red(tmp)) {
+		tmp->in_color = INTERVAL_BLACK;
+		parent->in_color = INTERVAL_RED;
+		if (n == 0) {
+			__rotate_left(parent, root);
+			tmp = parent->in_right;
+		} else {
+			__rotate_right(parent, root);
+			tmp = parent->in_left;
+		}
+	}
+	if (node_is_black_or_0(tmp->in_left) &&
+	    node_is_black_or_0(tmp->in_right)) {
+		tmp->in_color = INTERVAL_RED;
+		node = parent;
+		parent = node->in_parent;
+	} else {
+		if (n == 0) {
+			if (node_is_black_or_0(tmp->in_right)) {
+				struct interval_node *o_left;
+				o_left = tmp->in_left;
+				if (o_left)
+					o_left->in_color = INTERVAL_BLACK;
+				tmp->in_color = INTERVAL_RED;
+				__rotate_right(tmp, root);
+				tmp = parent->in_right;
+			}
+			tmp->in_color = parent->in_color;
+			parent->in_color = INTERVAL_BLACK;
+			if (tmp->in_right)
+				tmp->in_right->in_color = INTERVAL_BLACK;
+			__rotate_left(parent, root);
+			node = *root;
+			return 1;
+		} else {
+			if (node_is_black_or_0(tmp->in_left)) {
+				struct interval_node *o_right;
+				o_right = tmp->in_right;
+				if (o_right)
+					o_right->in_color = INTERVAL_BLACK;
+				tmp->in_color = INTERVAL_RED;
+				__rotate_left(tmp, root);
+				tmp = parent->in_left;
+			}
+			tmp->in_color = parent->in_color;
+			parent->in_color = INTERVAL_BLACK;
+			if (tmp->in_left)
+				tmp->in_left->in_color = INTERVAL_BLACK;
+			__rotate_right(parent, root);
+			node = *root;
+			return 1;
+		}
+	}
+	return 0;
+}
+
 static void interval_erase_color(struct interval_node *node,
 				 struct interval_node *parent,
 				 struct interval_node **root)
 {
-	struct interval_node *tmp;
 
 	while (node_is_black_or_0(node) && node != *root) {
 		if (parent->in_left == node) {
-			tmp = parent->in_right;
-			if (node_is_red(tmp)) {
-				tmp->in_color = INTERVAL_BLACK;
-				parent->in_color = INTERVAL_RED;
-				__rotate_left(parent, root);
-				tmp = parent->in_right;
-			}
-			if (node_is_black_or_0(tmp->in_left) &&
-			    node_is_black_or_0(tmp->in_right)) {
-				tmp->in_color = INTERVAL_RED;
-				node = parent;
-				parent = node->in_parent;
-			} else {
-				if (node_is_black_or_0(tmp->in_right)) {
-					struct interval_node *o_left;
-					o_left = tmp->in_left;
-					if (o_left)
-						o_left->in_color = INTERVAL_BLACK;
-					tmp->in_color = INTERVAL_RED;
-					__rotate_right(tmp, root);
-					tmp = parent->in_right;
-				}
-				tmp->in_color = parent->in_color;
-				parent->in_color = INTERVAL_BLACK;
-				if (tmp->in_right)
-					tmp->in_right->in_color = INTERVAL_BLACK;
-				__rotate_left(parent, root);
-				node = *root;
+			if (interval_set_color(node, parent, root, 0))
 				break;
-			}
 		} else {
-			tmp = parent->in_left;
-			if (node_is_red(tmp)) {
-				tmp->in_color = INTERVAL_BLACK;
-				parent->in_color = INTERVAL_RED;
-				__rotate_right(parent, root);
-				tmp = parent->in_left;
-			}
-			if (node_is_black_or_0(tmp->in_left) &&
-			    node_is_black_or_0(tmp->in_right)) {
-				tmp->in_color = INTERVAL_RED;
-				node = parent;
-				parent = node->in_parent;
-			} else {
-				if (node_is_black_or_0(tmp->in_left)) {
-					struct interval_node *o_right;
-					o_right = tmp->in_right;
-					if (o_right)
-						o_right->in_color = INTERVAL_BLACK;
-					tmp->in_color = INTERVAL_RED;
-					__rotate_left(tmp, root);
-					tmp = parent->in_left;
-				}
-				tmp->in_color = parent->in_color;
-				parent->in_color = INTERVAL_BLACK;
-				if (tmp->in_left)
-					tmp->in_left->in_color = INTERVAL_BLACK;
-				__rotate_right(parent, root);
-				node = *root;
+
+			if (interval_set_color(node, parent, root, 1))
 				break;
-			}
 		}
 	}
 	if (node)
-- 
1.7.9.5

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Dan Carpenter]

On Sat, Jan 11, 2014 at 04:30:33PM +0530, Monam Agarwal wrote:
> The function interval_erase_color() in lustre/ldlm/interval_tree.c is
> too long and can be refactored. Most of the statements are same for if
> and else conditions. I am passing a new variable n based on which the
> differences are recognised.
> 

"n" stands for number, but what is the it the number of?

regards,
dan carpenter

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Monam Agarwal]

I took n as a flag to decide whether parent->in_left == node is true
or not in the called function.
Should I use some other name for the flag.

On Sat, Jan 11, 2014 at 4:49 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Sat, Jan 11, 2014 at 04:30:33PM +0530, Monam Agarwal wrote:
>> The function interval_erase_color() in lustre/ldlm/interval_tree.c is
>> too long and can be refactored. Most of the statements are same for if
>> and else conditions. I am passing a new variable n based on which the
>> differences are recognised.
>>
>
> "n" stands for number, but what is the it the number of?
>
> regards,
> dan carpenter
>

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Dan Carpenter]

On Sat, Jan 11, 2014 at 04:56:44PM +0530, Monam Agarwal wrote:
> I took n as a flag to decide whether parent->in_left == node is true
> or not in the called function.

So "n" stands for "node"?

> Should I use some other name for the flag.


Yes.

regards,
dan carpenter

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Monam Agarwal]

On Sat, Jan 11, 2014 at 5:09 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Sat, Jan 11, 2014 at 04:56:44PM +0530, Monam Agarwal wrote:
>> I took n as a flag to decide whether parent->in_left == node is true
>> or not in the called function.
>
> So "n" stands for "node"?
>
>> Should I use some other name for the flag.
>
>
> Yes.
>

Will "flag" be a suitable name?

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Greg KH]

On Sat, Jan 11, 2014 at 05:14:35PM +0530, Monam Agarwal wrote:
> On Sat, Jan 11, 2014 at 5:09 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Sat, Jan 11, 2014 at 04:56:44PM +0530, Monam Agarwal wrote:
> >> I took n as a flag to decide whether parent->in_left == node is true
> >> or not in the called function.
> >
> > So "n" stands for "node"?
> >
> >> Should I use some other name for the flag.
> >
> >
> > Yes.
> >
> 
> Will "flag" be a suitable name?

Ick, no.  You don't want a "flag" to have to determine what the logic is
for a given function.  That just causes confusion and makes things
really hard to read and understand over time.

This whole function looks like a red/black tree, or something like that.
Shouldn't we just be using the in-kernel implementation of this?  And if
not, then you really need to get the feedback of the code's original
authors as you might be changing the algorithm in ways that could cause
big problems.

thanks,

greg k-h

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Xiong, Jinshan]

On Jan 13, 2014, at 11:56 PM, Dilger, Andreas <andreas.dilger@intel.com> wrote:

> 
> 
> Begin forwarded message:
> 
>> From: Greg KH <gregkh@linuxfoundation.org>
>> Subject: Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c
>> Date: January 11, 2014 at 1:33:58 PM MST
>> To: Monam Agarwal <monamagarwal123@gmail.com>
>> Cc: Dan Carpenter <dan.carpenter@oracle.com>, <devel@driverdev.osuosl.org>, <andreas.dilger@intel.com>, <peter.p.waskiewicz.jr@intel.com>, <linux-kernel@vger.kernel.org>, Rashika Kheria <rashika.kheria@gmail.com>
>> 
>> On Sat, Jan 11, 2014 at 05:14:35PM +0530, Monam Agarwal wrote:
>>> On Sat, Jan 11, 2014 at 5:09 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>> On Sat, Jan 11, 2014 at 04:56:44PM +0530, Monam Agarwal wrote:
>>>>> I took n as a flag to decide whether parent->in_left == node is true
>>>>> or not in the called function.
>>>> 
>>>> So "n" stands for "node"?
>>>> 
>>>>> Should I use some other name for the flag.
>>>> 
>>>> 
>>>> Yes.
>>>> 
>>> 
>>> Will "flag" be a suitable name?

I’d suggest `bool is_right_child’.

I’ve checked the patch and it looks good. There exists a unit test case for interval tree under lustre/tests/ named it_test.c, please compile it and verify your change.

Jinshan

>> 
>> Ick, no.  You don't want a "flag" to have to determine what the logic is
>> for a given function.  That just causes confusion and makes things
>> really hard to read and understand over time.
>> 
>> This whole function looks like a red/black tree, or something like that.
>> Shouldn't we just be using the in-kernel implementation of this?  And if
>> not, then you really need to get the feedback of the code's original
>> authors as you might be changing the algorithm in ways that could cause
>> big problems.
>> 
>> thanks,
>> 
>> greg k-h
> 
> Cheers, Andreas
> --
> Andreas Dilger
> Lustre Software Architect
> Intel Corporation
> 
> 
> 
> 
> 
> 

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Monam Agarwal]

On Tue, Jan 14, 2014 at 1:31 PM, Xiong, Jinshan <jinshan.xiong@intel.com> wrote:
>
> On Jan 13, 2014, at 11:56 PM, Dilger, Andreas <andreas.dilger@intel.com> wrote:
>
>>
>>
>> Begin forwarded message:
>>
>>> From: Greg KH <gregkh@linuxfoundation.org>
>>> Subject: Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c
>>> Date: January 11, 2014 at 1:33:58 PM MST
>>> To: Monam Agarwal <monamagarwal123@gmail.com>
>>> Cc: Dan Carpenter <dan.carpenter@oracle.com>, <devel@driverdev.osuosl.org>, <andreas.dilger@intel.com>, <peter.p.waskiewicz.jr@intel.com>, <linux-kernel@vger.kernel.org>, Rashika Kheria <rashika.kheria@gmail.com>
>>>
>>> On Sat, Jan 11, 2014 at 05:14:35PM +0530, Monam Agarwal wrote:
>>>> On Sat, Jan 11, 2014 at 5:09 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>>>>> On Sat, Jan 11, 2014 at 04:56:44PM +0530, Monam Agarwal wrote:
>>>>>> I took n as a flag to decide whether parent->in_left == node is true
>>>>>> or not in the called function.
>>>>>
>>>>> So "n" stands for "node"?
>>>>>
>>>>>> Should I use some other name for the flag.
>>>>>
>>>>>
>>>>> Yes.
>>>>>
>>>>
>>>> Will "flag" be a suitable name?
>
> I’d suggest `bool is_right_child’.
>
> I’ve checked the patch and it looks good. There exists a unit test case for interval tree under lustre/tests/ named it_test.c, please compile it and verify your change.
>
I am using tree from
http://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/.
There is no lustre/tests folder in my tree and no file named
it_test.c.
Kindly let me know the tree you are working from.

> Jinshan
>
>>>
>>> Ick, no.  You don't want a "flag" to have to determine what the logic is
>>> for a given function.  That just causes confusion and makes things
>>> really hard to read and understand over time.
>>>
>>> This whole function looks like a red/black tree, or something like that.
>>> Shouldn't we just be using the in-kernel implementation of this?  And if
>>> not, then you really need to get the feedback of the code's original
>>> authors as you might be changing the algorithm in ways that could cause
>>> big problems.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Cheers, Andreas
>> --
>> Andreas Dilger
>> Lustre Software Architect
>> Intel Corporation
>>
>>
>>
>>
>>
>>
>

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

[Xiong, Jinshan]

On Jan 14, 2014, at 3:09 AM, Monam Agarwal <monamagarwal123@gmail.com<mailto:monamagarwal123@gmail.com>> wrote:

On Tue, Jan 14, 2014 at 1:31 PM, Xiong, Jinshan <jinshan.xiong@intel.com<mailto:jinshan.xiong@intel.com>> wrote:

On Jan 13, 2014, at 11:56 PM, Dilger, Andreas <andreas.dilger@intel.com<mailto:andreas.dilger@intel.com>> wrote:



Begin forwarded message:

From: Greg KH <gregkh@linuxfoundation.org<mailto:gregkh@linuxfoundation.org>>
Subject: Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c
Date: January 11, 2014 at 1:33:58 PM MST
To: Monam Agarwal <monamagarwal123@gmail.com<mailto:monamagarwal123@gmail.com>>
Cc: Dan Carpenter <dan.carpenter@oracle.com<mailto:dan.carpenter@oracle.com>>, <devel@driverdev.osuosl.org<mailto:devel@driverdev.osuosl.org>>, <andreas.dilger@intel.com<mailto:andreas.dilger@intel.com>>, <peter.p.waskiewicz.jr@intel.com<mailto:peter.p.waskiewicz.jr@intel.com>>, <linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>>, Rashika Kheria <rashika.kheria@gmail.com<mailto:rashika.kheria@gmail.com>>

On Sat, Jan 11, 2014 at 05:14:35PM +0530, Monam Agarwal wrote:
On Sat, Jan 11, 2014 at 5:09 PM, Dan Carpenter <dan.carpenter@oracle.com<mailto:dan.carpenter@oracle.com>> wrote:
On Sat, Jan 11, 2014 at 04:56:44PM +0530, Monam Agarwal wrote:
I took n as a flag to decide whether parent->in_left == node is true
or not in the called function.

So "n" stands for "node"?

Should I use some other name for the flag.


Yes.


Will "flag" be a suitable name?

I’d suggest `bool is_right_child’.

I’ve checked the patch and it looks good. There exists a unit test case for interval tree under lustre/tests/ named it_test.c, please compile it and verify your change.

I am using tree from
http://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/.
There is no lustre/tests folder in my tree and no file named
it_test.c.
Kindly let me know the tree you are working from.

I’m using git tree git://git.hpdd.intel.com/fs/lustre-release.git<http://git.hpdd.intel.com/?p=fs/lustre-release.git>

there are also some useful information about compiling and running the code: https://wiki.hpdd.intel.com/display/PUB/Testing+a+Lustre+filesystem, usually I download a patched kernel rpm so that I can save a lot of time from building the kernel myself. Go to build.whamcloud.com<http://build.whamcloud.com> and look for lustre-master, click in and choose server side package based on your distro.

If you’re interested in contributing code to lustre, here are some useful links about creating the patch, review the patch, pass regression test, etc: https://wiki.hpdd.intel.com/display/PUB/Lustre+Development

Jinshan


Jinshan


Ick, no.  You don't want a "flag" to have to determine what the logic is
for a given function.  That just causes confusion and makes things
really hard to read and understand over time.

This whole function looks like a red/black tree, or something like that.
Shouldn't we just be using the in-kernel implementation of this?  And if
not, then you really need to get the feedback of the code's original
authors as you might be changing the algorithm in ways that could cause
big problems.

thanks,

greg k-h

Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel Corporation