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

[Fabian Frederick <fabf@xxxxxxxxx>]

> On 04 June 2015 at 07:01 Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
>
> On Wed, May 27, 2015 at 02:57:35PM -0700, Andrew Morton wrote:
> > On Wed, 27 May 2015 21:15:30 +0200 Fabian Frederick <fabf@xxxxxxxxx> wrote:
> >
> > > This reverts commit 9ef7db7f38d0
> > > ("ufs: fix deadlocks introduced by sb mutex merge")
> > > That patch tried to solve
> > >Â Â ÂCommit 0244756edc4b98c
> > >Â Â Â("ufs: sb mutex merge + mutex_destroy")
> > > which is itself partially reverted due to multiple deadlocks
> >
> > This is all very vague. The changelogs are missing any description of
> > the deadlocks: how they are triggered, why they occur. And there's no
> > description of how the patches fix these deadlocks. And as we're
> > reverting a bunch of things one wonders whether the problems which the
> > now-reverted patches fixed are being reintroduced.
> >
> > Has anyone (Ian?) confirmed that the fs works OK with these patches?
>
> Folks, how about we figure out what's really being protected by that
> mutex? IIRC, the main irregularity about ufs is the need to deal with
> growing the partial final block - unlike e.g ext2, ufs has differently-sized
> blocks and fragments. Basically, it's tail-packing - short files have
> the last used direct pointer refer to a group of adjacent fragments that
> doesn't have to be block-aligned or fill the entire block. They can't
> cross the disk block boundary and write might have to reallocate the partial
> block.
>
> So we need
>Â Â Â Â* 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)
>
> Looks like we ought to add ->truncate_mutex and shove lock_ufs() calls
> all way down into balloc.c (and ialloc.c for inode allocations)...

If we look at linux-next with the original mutex behavior restored,
mutex/spinlocks are the following:

struct ufs_sb_info{

    struct mutex mutex; (lock_ufs()/unlock_ufs())
    struct mutex s_lock; (mutex_lock()/mutex_unlock())
    spinlock_t work_lock; /* protects sync_work and work_queued */

}

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 ? That would involve dropping the two linux-next
reverts in ufs.

Regards,
Fabian

Attachment: ufsmutex2
Description: Binary data

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