Re: [PATCH 1/2 linux-next] Revert "ufs: fix deadlocks introduced by sb mutex merge"

[Fabian Frederick <fabf@skynet.be>]

> On 05 June 2015 at 20:50 Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
>
> On Fri, Jun 05, 2015 at 06:27:01PM +0200, Fabian Frederick wrote:
>
> > You're asking to remove lock_ufs() in allocation and replace it by
> > truncate_mutex. I guess you're talking about doing that on current rc
> > (without s_lock restored).
> >
> > I tried a quick patch on rc trying to convert lock_ufs()/unlock_ufs()
> > with per inode truncate_mutex (see attachment).
> > Is it going the right direction ?
>
> Nope.  First of all, allocation should be protected by fs-wide mutex, for
> obvious reasons.  What ->truncate_mutex is protecting (outside of that
> one) is modifications of block pointers in inode and in indirect blocks.
>
> Check what ext2 is doing.  It tries to follow pointers without taking
> a lock.  If it succeeds, fine, no allocation is needed, just map it.
> If it fails, it grabs ->truncate_mutex, follows pointers again (checking
> the chain it followed before grabbing the lock first, in hope it has
> remained valid) and does allocations and block pointer modifications,
> with ->truncate_mutex making sure that nobody else would touch those
> under it.  And on truncate side it deals with the page into which EOF
> will fall, then (still holding ->i_mutex) does setting ->i_size and
> eviction of pages beyond EOF (by truncate_setsize()), then does freeing
> the blocks past the new EOF.  Which is where it grabs ->truncate_mutex
> (inside __ext2_truncate_blocks()).  And it doesn't need to do those
> insane retries - on get_block side we need to redo the pointer chasing
> after having taken ->truncate_mutex (when it decides that it might need
> to do allocation, after all), but that's done once; truncate side just
> grabs ->truncate_mutex as soon as it gets to __ext2_truncate_blocks(),
> which means that nobody can change anything under it.
>
> Actual allocation/freeing of blocks (as well as that of inodes) is done
> under fs-wide mutex, nesting inside the rest.
>
> Basically, we have
>       i_mutex: file size changes, contents-affecting syscalls.  Per-inode.
>       truncate_mutex: block pointers changes.  Per-inode.
>       s_lock: block and inode bitmaps changes.  Per-filesystem.
>
> For UFS it's slightly more complicated due to tail packing they are doing for
> short files, but most of that complexity is due to having that stuff handled
> way too deep in call chain.

In your two explanations, there's only a place for one sb mutex:
"
i_mutex: file size changes, contents-affecting syscalls.  Per-inode.
truncate_mutex: block pointers changes.  Per-inode.
s_lock: block and inode bitmaps changes.  Per-filesystem.
"
"
per-page exclusion for reallocation time (normal page locks are doing that)
per-fs exclusion for block and fragment allocations (->s_lock?)
per-fs exclusion for inode allocations (->s_lock?)
per-inode exclusion for mapping changes (a-la ext2 truncate_mutex)
per-directory exclusion for contents access (->i_mutex gives that)       
"

Meanwhile current linux-next has 2 in fs/ufs/ufs.h: struct ufs_sb_info

struct mutex mutex
struct mutex s_lock

So commit 0244756edc4b98c129e "ufs: sb mutex merge + mutex_destroy")
was finally not a bad thing but maybe it removed the bad one ?

Regards,
Fabian

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel