Re: [patch] f2fs: use _safe() version of list_for_each

[Julia Lawall <julia.lawall@lip6.fr>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1744 bytes --]

On Mon, 21 Jan 2013, Namjae Jeon wrote:

> 2013/1/21, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> > 2013-01-21 (월), 00:32 -0800, Dmitry Torokhov:
> >> On Sun, Jan 20, 2013 at 06:02:58PM +0300, Dan Carpenter wrote:
> >> > This is calling list_del() inside a loop which is a problem when we try
> >> > move to the next item on the list.  I've converted it to use the _safe
> >> > version.  And also, as a cleanup, I've converted it to use
> >> > list_for_each_entry instead of list_for_each.
> >> >
> >> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >> > ---
> >> > Static analysis stuff.  Untested.  Please review carefully.
> >>
> >> Makes sense to me.
> >>
> >> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>
> >
> > No doubt, applied.
> > Thanks,
> I agree in cases – where we will have chances of parallel access and
> modification to the linked we should use list_for_each_entry_safe()
> But my point was related with this code change case in the patch.
> We will have path like this:
> f2fs_fill_super->recover_fsync_data->destroy_fsync_dnodes()
> From this calling path – there can only be a single caller at any
> given time. So, we will not have the case of having parallel access to
> the list which is local to recover_fsync_data() and destroyed on exit
> from this function.
> From the real issue point of view – this does not looks convincing to
> me why expensive _safe fucntion should used.

The need for the safe version has nothing to do with parallelism.  It has
to do with whether the loop deletes some of the elements in the list.
The non-safe list mechanism uses the current element to get to the next
element.  If the current element has been deleted, it cannot get to the
next one.

julia