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

Xiong, Jinshan jinshan.xiong at intel.com
Tue Jan 14 17:41:58 UTC 2014


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

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

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



Begin forwarded message:

From: Greg KH <gregkh at linuxfoundation.org<mailto:gregkh at 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 at gmail.com<mailto:monamagarwal123 at gmail.com>>
Cc: Dan Carpenter <dan.carpenter at oracle.com<mailto:dan.carpenter at oracle.com>>, <devel at driverdev.osuosl.org<mailto:devel at driverdev.osuosl.org>>, <andreas.dilger at intel.com<mailto:andreas.dilger at intel.com>>, <peter.p.waskiewicz.jr at intel.com<mailto:peter.p.waskiewicz.jr at intel.com>>, <linux-kernel at vger.kernel.org<mailto:linux-kernel at vger.kernel.org>>, Rashika Kheria <rashika.kheria at gmail.com<mailto:rashika.kheria at 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 at oracle.com<mailto:dan.carpenter at 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



More information about the devel mailing list