[Top] [All Lists]

Re: [PATCH 0/6] xfs: discombobulate sb updates and kill xfs_vnode.h

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 0/6] xfs: discombobulate sb updates and kill xfs_vnode.h
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 1 Aug 2014 09:01:36 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20140731171655.GA30301@xxxxxxxxxxxxx>
References: <1406791995-14723-1-git-send-email-david@xxxxxxxxxxxxx> <20140731171655.GA30301@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Jul 31, 2014 at 10:16:55AM -0700, Christoph Hellwig wrote:
> On Thu, Jul 31, 2014 at 05:33:09PM +1000, Dave Chinner wrote:
> > Hi folks,
> > 
> > Really two patch series in one. The first two patches remove the
> > bitfield based superblock update method and replace it with a simple
> > "update and log everything" operation. Superblock updates are
> > now relatively rare so there's no need to optimise for single field
> > updates. This patchset removes all that complex code and makes
> > everything nice and simple.
> Is there any deeper rationale why you want to get rid of it?

Tangential - cleaning up the mess of separate project quota inode
support. We do lots of dancing around with bit flags and updates in
different functions, and it really just complicates the code.
The recent problems with the quota flags getting
screwed up by repair due not using the sb-to-disk code properly
in userspace was the initial source of the problem - it's just an
unmaintainable PITA that leads to bugs. So the first step to fixing
that is removing all of the unnecessary obfuscation and complexity
from the kernel code, then port it over to userspace(*).

Besides, when we log a single field in the superblock, we're really
logging the surrounding 128 byte chunk, so we really are logging
most of the superbock on every change right now. And if we want to
move to single regions for logged buffers, then we'll be logging the
entire sb every time anyway. So, really, it makes no sense to do
this special bitfield based update and logging stuff anymore.



(*) I haven't posted the "trash all quota state" patch set for
repair yet - given that repair doesn't validate the quota inode
contents, and we quotacheck unconditionally after a repair
run, there's no point trying to check or repair quota state or
inodes at all - just trash them and let the kernel rebuild it from
scratch on the next mount....
Dave Chinner

<Prev in Thread] Current Thread [Next in Thread>