Re: [PATCH 5/5] blkcg: remove blkio_group->stats_lock

[Tejun Heo]

Hey, Vivek.

On Fri, Mar 09, 2012 at 12:15:23PM -0500, Vivek Goyal wrote:
> On Thu, Mar 08, 2012 at 10:54:00AM -0800, Tejun Heo wrote:
> > With recent plug merge updates, all non-percpu stat updates happen
> > under queue_lock making stats_lock unnecessary to synchronize stat
> > updates.  The only synchronization necessary is stat reading, which
> > can be done using u64_stats_sync instead.
> > 
> > This patch removes blkio_group->stats_lock and adds
> > blkio_group_stats->syncp for reader synchronization.
> 
> Good to see stat_lock going away. That lock was confusing as we were doing
> updates under queue_lock anyway. One less lock to think about now.
> 
> This stat code is messy though. All time related stat, maintaining flags,
> dividing stats between read/write, sync/async. I think we are maintaining
> way too much of stat. (/me wished there was a way to get rid of some of
> the stats and make code simpler).

The biggest problem I'm having with the stat code is that it almost
mandates spaghettiness.  This stems both from the limitations of
cgroup files and the implementation.  blkcg needs to declare all
possible files upfront on creating cgroup directories, so it needs to
know all files which may be used by any policy implementation.  This
leads another layer of indirection in blkcg core and when a policy
wants to create a new file (including stat), it has to add it to the
blkcg core and then walk back.  This is inherently nasty.  I'm trying
to see whether cgroup can be updated to allow dynamic file
creation/removal so that policies can manage their own files instead
of having to dump everything into blkcg core.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/