Re: Ext3 deadlocks with quota

[Andrew Morton <akpm@digeo.com>]

Jan Kara <jack@ucw.cz> wrote:
>
>   Hello,
> 
>   I'm sending a patch which should fix the deadlocks with quota_sync()
> racing with create() (lock inversion on journal_start and dqio_sem).
> Please could you check that the patch is correct?
> 
> 						Thanks
> 							Honza

Sorry for the delay...

> diff -ru linux-2.5.64/fs/dquot.c linux-2.5.64-ext3deadlock/fs/dquot.c
> --- linux-2.5.64/fs/dquot.c	Wed Mar  5 04:29:31 2003
> +++ linux-2.5.64-ext3deadlock/fs/dquot.c	Wed Mar 19 11:23:41 2003
> @@ -326,7 +326,7 @@
>  		if (!dquot_dirty(dquot))
>  			continue;
>  		spin_unlock(&dq_list_lock);
> -		commit_dqblk(dquot);
> +		sb->dq_op->sync_dquot(dquot);
>  		goto restart;

Do we know that the fs has a non-null sb->dq_op here?

>  	}
>  	spin_unlock(&dq_list_lock);
> @@ -1072,7 +1072,8 @@
>  	.alloc_inode	= dquot_alloc_inode,
>  	.free_space	= dquot_free_space,
>  	.free_inode	= dquot_free_inode,
> -	.transfer	= dquot_transfer
> +	.transfer	= dquot_transfer,
> +	.sync_dquot	= commit_dqblk
>  };
>  
>  static inline void set_enable_flags(struct quota_info *dqopt, int type)
> diff -ru linux-2.5.64/fs/ext3/super.c linux-2.5.64-ext3deadlock/fs/ext3/super.c
> --- linux-2.5.64/fs/ext3/super.c	Wed Mar  5 04:29:35 2003
> +++ linux-2.5.64-ext3deadlock/fs/ext3/super.c	Wed Mar 19 11:28:04 2003
> @@ -20,6 +20,7 @@
>  #include <linux/module.h>
>  #include <linux/string.h>
>  #include <linux/fs.h>
> +#include <linux/quotaops.h>
>  #include <linux/time.h>
>  #include <linux/jbd.h>
>  #include <linux/ext3_fs.h>
> @@ -494,6 +495,10 @@
>  # define ext3_clear_inode NULL
>  #endif
>  
> +#ifdef CONFIG_QUOTA
> +static struct dquot_operations ext3_qops;
> +#endif
> +

I'd remove this ifdef - it's only 32 bytes...

>  static struct super_operations ext3_sops = {
>  	.alloc_inode	= ext3_alloc_inode,
>  	.destroy_inode	= ext3_destroy_inode,
> @@ -1262,6 +1267,9 @@
>  	 */
>  	sb->s_op = &ext3_sops;
>  	sb->s_export_op = &ext3_export_ops;
> +#ifdef CONFIG_QUOTA
> +	sb->dq_op = &ext3_qops;
> +#endif

And this one.

>  	INIT_LIST_HEAD(&sbi->s_orphan); /* unlinked but open files */
>  
>  	sb->s_root = 0;
> @@ -1897,6 +1905,53 @@
>  	return 0;
>  }
>  
> +#ifdef CONFIG_QUOTA
> +/* Helper function for writing quotas on sync - we need to start transaction before quota file
> + * is locked for write. Otherwise the are possible deadlocks:
> + * Process 1                         Process 2
> + * ext3_create()                     quota_sync()
> + *   journal_start()                   write_dquot()
> + *   DQUOT_INIT()                        down(dqio_sem)
> + *     down(dqio_sem)                    journal_start()
> + *
> + */
> +

OK.

> +#define EXT3_OLD_QFMT_BLOCKS 2
> +#define EXT3_V0_QFMT_BLOCKS 6
> +
> +static int ext3_sync_dquot(struct dquot *dquot)
> +{
> +	int nblocks, ret;
> +	handle_t *handle;
> +	struct quota_info *dqops = sb_dqopt(dquot->dq_sb);
> +	struct inode *qinode;
> +
> +	switch (dqops->info[dquot->dq_type].dqi_format->qf_fmt_id) {
> +		case QFMT_VFS_OLD:
> +			nblocks = EXT3_OLD_QFMT_BLOCKS;
> +			break;
> +		case QFMT_VFS_V0:
> +			nblocks = EXT3_V0_QFMT_BLOCKS;
> +			break;
> +		default:
> +			nblocks = EXT3_MAX_TRANS_DATA;
> +	}
> +	lock_kernel();
> +	qinode = dqops->files[dquot->dq_type]->f_dentry->d_inode;
> +	handle = ext3_journal_start(qinode, nblocks);
> +	if (IS_ERR(handle)) {
> +		unlock_kernel();
> +		return PTR_ERR(handle);
> +	}
> +	unlock_kernel();
> +	ret = dquot_operations.sync_dquot(dquot);

Shouldn't this be ext3_qops.sync_dquot()?


> +	lock_kernel();
> +	ext3_journal_stop(handle, qinode);

This should be

	ret = ext3_journal_stop()

> +	unlock_kernel();
> +	return ret;
> +}
> +#endif
> +
>  static struct super_block *ext3_get_sb(struct file_system_type *fs_type,
>  	int flags, char *dev_name, void *data)
>  {
> @@ -1919,6 +1974,10 @@
>  	err = init_inodecache();
>  	if (err)
>  		goto out1;
> +#ifdef CONFIG_QUOTA
> +	memcpy(&ext3_qops, &dquot_operations, sizeof(ext3_qops));
> +	ext3_qops.sync_dquot = ext3_sync_dquot;
> +#endif

I think it would be cleaner to call a new

	init_dquot_operations(&ext3_qops);

here.

If you violently disagree, then we'll need to export dquot_operations to
modules.  init_dquot_operations() would need to be exported too...


>          err = register_filesystem(&ext3_fs_type);
>  	if (err)
>  		goto out;
> diff -ru linux-2.5.64/include/linux/quota.h linux-2.5.64-ext3deadlock/include/linux/quota.h
> --- linux-2.5.64/include/linux/quota.h	Wed Mar  5 04:29:32 2003
> +++ linux-2.5.64-ext3deadlock/include/linux/quota.h	Wed Mar 19 11:23:41 2003
> @@ -250,6 +250,7 @@
>  	void (*free_space) (struct inode *, qsize_t);
>  	void (*free_inode) (const struct inode *, unsigned long);
>  	int (*transfer) (struct inode *, struct iattr *);
> +	int (*sync_dquot) (struct dquot *);
>  };
>  
>  /* Operations handling requests from userspace */
> 

Apart from that, if it fixes the deadlock and if Stephen is OK with it, let's
run with it.

Thanks.