<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Exchange Server">
<!-- converted from text --><style><!-- .EmailQuote { margin-left: 1pt; padding-left: 4pt; border-left: #800000 2px solid; } --></style>
</head>
<body>
<meta content="text/html; charset=UTF-8">
<style type="text/css" style="">
<!--
p
{margin-top:0;
margin-bottom:0}
-->
</style>
<div dir="ltr">
<div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Arial,Helvetica,sans-serif">
<p>I would also dispute the logic in this particular Coccinelle script more generally.</p>
<p><br>
</p>
<p>list_for_each_entry_safe and while(list_empty(&list) == 0) (or, more clearly, while(!list_empty(&list))) are NOT equivalent.</p>
<p><br>
</p>
<p>Unless I've misunderstood the "for" loop used, that will run once for each item in the list, 'while' will run until the list is empty. They just aren't the same thing, and can't be swapped one for one in general. Sure, sometimes the usage is equivalent,
but this coccinelle script can at most flag candidates for a change (which must be checked), it can't be used to just generate patches.</p>
<p><br>
</p>
<p>- Patrick</p>
</div>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> lustre-devel <lustre-devel-bounces@lists.lustre.org> on behalf of James Simmons <jsimmons@infradead.org><br>
<b>Sent:</b> Monday, March 6, 2017 9:20:49 AM<br>
<b>To:</b> simran singhal<br>
<b>Cc:</b> devel@driverdev.osuosl.org; gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; oleg.drokin@intel.com; outreachy-kernel@googlegroups.com; lustre-devel@lists.lustre.org<br>
<b>Subject:</b> Re: [lustre-devel] [PATCH 5/5] staging: lustre: osc_page.c: Use list_for_each_entry_safe</font>
<div> </div>
</div>
</div>
<font size="2"><span style="font-size:10pt;">
<div class="PlainText"><br>
> Doubly linked lists which are iterated using list_empty<br>
> and list_entry macros have been replaced with list_for_each_entry_safe<br>
> macro.<br>
> This makes the iteration simpler and more readable.<br>
> <br>
> This patch replaces the while loop containing list_empty and list_entry<br>
> with list_for_each_entry_safe.<br>
> <br>
> This was done with Coccinelle.<br>
> <br>
> @@<br>
> expression E1;<br>
> identifier I1, I2;<br>
> type T;<br>
> iterator name list_for_each_entry_safe;<br>
> @@<br>
> <br>
> T *I1;<br>
> + T *tmp;<br>
> ...<br>
> - while (list_empty(&E1) == 0)<br>
> + list_for_each_entry_safe (I1, tmp, &E1, I2)<br>
> {<br>
> ...when != T *I1;<br>
> - I1 = list_entry(E1.next, T, I2);<br>
> ...<br>
> }<br>
> <br>
> Signed-off-by: simran singhal <singhalsimran0@gmail.com><br>
<br>
NAK!!!!!!<br>
<br>
This change was reverted in commit <br>
<br>
cd15dd6ef4ea11df87f717b8b1b83aaa738ec8af<br>
<br>
Doing these while (list_empty(..)) to list_for_entry...<br>
are not simple changes and have broken things in lustre<br>
before. Unless you really understand the state machine of<br>
the lustre code I don't recommend these kinds of change<br>
for lustre.<br>
<br>
> ---<br>
> drivers/staging/lustre/lustre/osc/osc_page.c | 11 ++++-------<br>
> 1 file changed, 4 insertions(+), 7 deletions(-)<br>
> <br>
> diff --git a/drivers/staging/lustre/lustre/osc/osc_page.c b/drivers/staging/lustre/lustre/osc/osc_page.c<br>
> index ed8a0dc..e8b974f 100644<br>
> --- a/drivers/staging/lustre/lustre/osc/osc_page.c<br>
> +++ b/drivers/staging/lustre/lustre/osc/osc_page.c<br>
> @@ -542,6 +542,7 @@ long osc_lru_shrink(const struct lu_env *env, struct client_obd *cli,<br>
> struct cl_object *clobj = NULL;<br>
> struct cl_page **pvec;<br>
> struct osc_page *opg;<br>
> + struct osc_page *tmp;<br>
> int maxscan = 0;<br>
> long count = 0;<br>
> int index = 0;<br>
> @@ -572,7 +573,7 @@ long osc_lru_shrink(const struct lu_env *env, struct client_obd *cli,<br>
> if (force)<br>
> cli->cl_lru_reclaim++;<br>
> maxscan = min(target << 1, atomic_long_read(&cli->cl_lru_in_list));<br>
> - while (!list_empty(&cli->cl_lru_list)) {<br>
> + list_for_each_entry_safe(opg, tmp, &cli->cl_lru_list, ops_lru) {<br>
> struct cl_page *page;<br>
> bool will_free = false;<br>
> <br>
> @@ -582,8 +583,6 @@ long osc_lru_shrink(const struct lu_env *env, struct client_obd *cli,<br>
> if (--maxscan < 0)<br>
> break;<br>
> <br>
> - opg = list_entry(cli->cl_lru_list.next, struct osc_page,<br>
> - ops_lru);<br>
> page = opg->ops_cl.cpl_page;<br>
> if (lru_page_busy(cli, page)) {<br>
> list_move_tail(&opg->ops_lru, &cli->cl_lru_list);<br>
> @@ -1043,6 +1042,7 @@ unsigned long osc_cache_shrink_scan(struct shrinker *sk,<br>
> {<br>
> struct client_obd *stop_anchor = NULL;<br>
> struct client_obd *cli;<br>
> + struct client_obd *tmp;<br>
> struct lu_env *env;<br>
> long shrank = 0;<br>
> u16 refcheck;<br>
> @@ -1059,10 +1059,7 @@ unsigned long osc_cache_shrink_scan(struct shrinker *sk,<br>
> return SHRINK_STOP;<br>
> <br>
> spin_lock(&osc_shrink_lock);<br>
> - while (!list_empty(&osc_shrink_list)) {<br>
> - cli = list_entry(osc_shrink_list.next, struct client_obd,<br>
> - cl_shrink_list);<br>
> -<br>
> + list_for_each_entry_safe(cli, tmp, &osc_shrink_list, cl_shrink_list) {<br>
> if (!stop_anchor)<br>
> stop_anchor = cli;<br>
> else if (cli == stop_anchor)<br>
> -- <br>
> 2.7.4<br>
> <br>
> <br>
_______________________________________________<br>
lustre-devel mailing list<br>
lustre-devel@lists.lustre.org<br>
<a href="http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org">http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org</a><br>
</div>
</span></font>
</body>
</html>