On Fri, Jan 31, 2014 at 1:04 PM, Andriy Gapon <[email protected]> wrote:
> on 31/01/2014 17:58 Matthew Ahrens said the following: > > > > > > > > On Thu, Jan 30, 2014 at 11:44 PM, Andriy Gapon <[email protected] > > <mailto:[email protected]>> wrote: > > > > on 31/01/2014 07:57 Matthew Ahrens said the following: > > > On Thu, Jan 30, 2014 at 2:02 AM, Andriy Gapon <[email protected] > > <mailto:[email protected]> > > > <mailto:[email protected] <mailto:[email protected]>>> 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. > > Ah, I see. Yes, it would seem that the ARC_L2_WRITE_HEAD should only exist while l2arc_write_buffers() is running, and therefore l2arc_evict() should assert that it does not see the WRITE_HEAD buffer. --matt
_______________________________________________ developer mailing list [email protected] http://lists.open-zfs.org/mailman/listinfo/developer
