on 31/01/2014 17:58 Matthew Ahrens said the following: > > > > On Thu, Jan 30, 2014 at 11:44 PM, Andriy Gapon <a...@freebsd.org > <mailto:a...@freebsd.org>> wrote: > > on 31/01/2014 07:57 Matthew Ahrens said the following: > > On Thu, Jan 30, 2014 at 2:02 AM, Andriy Gapon <a...@freebsd.org > <mailto:a...@freebsd.org> > > <mailto:a...@freebsd.org <mailto:a...@freebsd.org>>> wrote: > > > > > > I can not figure out how the following code actually works. > > Probably I am missing something in the big picture (again). > > > > if (HDR_L2_WRITE_HEAD(ab)) { > > /* > > * We hit a write head node. Leave it for > > * l2arc_write_done(). > > */ > > list_remove(buflist, ab); > > mutex_exit(hash_lock); > > continue; > > } > > > > So, the write head is left in memory, but it is still removed from > l2ad_buflist. > > Supposing there is a corresponding L2 write zio in progress there > will > be a call > > to l2arc_write_done() with l2wcb_head pointing to the head. > > Wouldn't > > list_prev(buflist, head) > > result in an illegal memory access if head is not on buflist? > > > > > > Yes, it would. Thankfully, we don't call list_prev() after removing > it. The > > loop in l2arc_evict() begins with: > > > > for (ab = list_tail(buflist); ab; ab = ab_prev) { > > ab_prev = list_prev(buflist, ab); > > Apologies for not being clear, I actually meant the loop in > l2arc_write_done(). > > > l2arc_buflist_mtx should prevent that.
Umm... l2arc_write_buffers() creates the head object and puts it on the list. Then it creates a parent zio and the head and the list are tucked away in a callback data associated with the zio. Then l2arc_write_buffers() drops l2arc_buflist_mtx and calls zio_wait on the zio. Let's assume that then l2arc_evict() is called and it removes the head from the list. This is done under l2arc_buflist_mtx protection, but that's beside the point. After that the zio is done and l2arc_write_done() callback is called. It acquires l2arc_buflist_mtx and tries to walk the list starting with head that was stored in the callback data. But the head is not on the list... This was my concern. But now that I have written the above I see that l2arc_write_buffers() and l2arc_evict() are called sequentially from l2arc_feed_thread() and thus l2arc_write_done() can't run concurrently with l2arc_evict(). So, this is what I have missed in the big picture :-) I am trying to understand now if we can assert that l2arc_evict() should never encounter ARC_L2_WRITE_HEAD marked buffer on a list. Although, there is another call l2arc_evict() in l2arc_remove_vdev(). But it looks like that should be protected by spa_config lock. -- Andriy Gapon _______________________________________________ developer mailing list developer@open-zfs.org http://lists.open-zfs.org/mailman/listinfo/developer
