Re: [PATCH 5/5] staging: lustre: osc_page.c: Use list_for_each_entry_safe

[Dilger, Andreas]

On Mar 6, 2017, at 08:20, James Simmons <jsimmons@xxxxxxxxxxxxx> wrote:
> 
>> 
>> Doubly linked lists which are  iterated  using list_empty
>> and list_entry macros have been replaced with list_for_each_entry_safe
>> macro.
>> This makes the iteration simpler and more readable.
>> 
>> This patch replaces the while loop containing list_empty and list_entry
>> with list_for_each_entry_safe.
>> 
>> This was done with Coccinelle.
>> 
>> @@
>> expression E1;
>> identifier I1, I2;
>> type T;
>> iterator name list_for_each_entry_safe;
>> @@
>> 
>> T *I1;
>> + T *tmp;
>> ...
>> - while (list_empty(&E1) == 0)
>> + list_for_each_entry_safe (I1, tmp, &E1, I2)
>> {
>> ...when != T *I1;
>> - I1 = list_entry(E1.next, T, I2);
>> ...
>> }
>> 
>> Signed-off-by: simran singhal <singhalsimran0@xxxxxxxxx>
> 
> NAK!!!!!!
> 
> This change was reverted in commit 
> 
> cd15dd6ef4ea11df87f717b8b1b83aaa738ec8af
> 
> Doing these while (list_empty(..)) to list_for_entry...
> are not simple changes and have broken things in lustre
> before. Unless you really understand the state machine of
> the lustre code I don't recommend these kinds of change
> for lustre.

It may be useful to add a comment to these cases where the while() loop cannot be
replaced by list_for_each_entry_safe() (with details of why that is the case) to
avoid such optimization attempts again in the future.

Cheers, Andreas

> 
>> ---
>> drivers/staging/lustre/lustre/osc/osc_page.c | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/osc/osc_page.c b/drivers/staging/lustre/lustre/osc/osc_page.c
>> index ed8a0dc..e8b974f 100644
>> --- a/drivers/staging/lustre/lustre/osc/osc_page.c
>> +++ b/drivers/staging/lustre/lustre/osc/osc_page.c
>> @@ -542,6 +542,7 @@ long osc_lru_shrink(const struct lu_env *env, struct client_obd *cli,
>> 	struct cl_object *clobj = NULL;
>> 	struct cl_page **pvec;
>> 	struct osc_page *opg;
>> +	struct osc_page *tmp;
>> 	int maxscan = 0;
>> 	long count = 0;
>> 	int index = 0;
>> @@ -572,7 +573,7 @@ long osc_lru_shrink(const struct lu_env *env, struct client_obd *cli,
>> 	if (force)
>> 		cli->cl_lru_reclaim++;
>> 	maxscan = min(target << 1, atomic_long_read(&cli->cl_lru_in_list));
>> -	while (!list_empty(&cli->cl_lru_list)) {
>> +	list_for_each_entry_safe(opg, tmp, &cli->cl_lru_list, ops_lru) {
>> 		struct cl_page *page;
>> 		bool will_free = false;
>> 
>> @@ -582,8 +583,6 @@ long osc_lru_shrink(const struct lu_env *env, struct client_obd *cli,
>> 		if (--maxscan < 0)
>> 			break;
>> 
>> -		opg = list_entry(cli->cl_lru_list.next, struct osc_page,
>> -				 ops_lru);
>> 		page = opg->ops_cl.cpl_page;
>> 		if (lru_page_busy(cli, page)) {
>> 			list_move_tail(&opg->ops_lru, &cli->cl_lru_list);
>> @@ -1043,6 +1042,7 @@ unsigned long osc_cache_shrink_scan(struct shrinker *sk,
>> {
>> 	struct client_obd *stop_anchor = NULL;
>> 	struct client_obd *cli;
>> +	struct client_obd *tmp;
>> 	struct lu_env *env;
>> 	long shrank = 0;
>> 	u16 refcheck;
>> @@ -1059,10 +1059,7 @@ unsigned long osc_cache_shrink_scan(struct shrinker *sk,
>> 		return SHRINK_STOP;
>> 
>> 	spin_lock(&osc_shrink_lock);
>> -	while (!list_empty(&osc_shrink_list)) {
>> -		cli = list_entry(osc_shrink_list.next, struct client_obd,
>> -				 cl_shrink_list);
>> -
>> +	list_for_each_entry_safe(cli, tmp, &osc_shrink_list, cl_shrink_list) {
>> 		if (!stop_anchor)
>> 			stop_anchor = cli;
>> 		else if (cli == stop_anchor)
>> -- 
>> 2.7.4

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