* Ext3 deadlocks with quota
@ 2003-03-19 17:31 Jan Kara
2003-04-09 9:42 ` Andrew Morton
0 siblings, 1 reply; 2+ messages in thread
From: Jan Kara @ 2003-03-19 17:31 UTC (permalink / raw)
To: akpm, sct; +Cc: linux-fsdevel
[-- Attachment #1: Type: text/plain, Size: 228 bytes --]
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
[-- Attachment #2: quota-2.5.64-1-ext3deadlock.diff --]
[-- Type: text/plain, Size: 3835 bytes --]
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;
}
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
+
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
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()
+ *
+ */
+
+#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);
+ lock_kernel();
+ ext3_journal_stop(handle, qinode);
+ 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
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 */
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: Ext3 deadlocks with quota
2003-03-19 17:31 Ext3 deadlocks with quota Jan Kara
@ 2003-04-09 9:42 ` Andrew Morton
0 siblings, 0 replies; 2+ messages in thread
From: Andrew Morton @ 2003-04-09 9:42 UTC (permalink / raw)
To: Jan Kara; +Cc: sct, linux-fsdevel
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2003-04-09 9:30 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2003-03-19 17:31 Ext3 deadlocks with quota Jan Kara
2003-04-09 9:42 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).