From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [patch v3 1/3] reiserfs: locking, push write lock out of xattr code Date: Wed, 7 Aug 2013 22:29:06 +0200 Message-ID: <20130807202906.GH26516@quack.suse.cz> References: <20130807163515.986450231@suse.com> <20130807163550.323947902@suse.com> Mime-Version: 1.0 Return-path: Content-Disposition: inline In-Reply-To: <20130807163550.323947902@suse.com> Sender: reiserfs-devel-owner@vger.kernel.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Jeff Mahoney Cc: reiserfs-devel@vger.kernel.org, jack@suse.cz On Wed 07-08-13 12:35:16, Jeff Mahoney wrote: > The reiserfs xattr code doesn't need the write lock and sleeps all over > the place. We can simplify the locking by releasing it and reacquiring > after the xattr call. The patch looks good so I'll merge it once comment to patch 2/3 gets sorted out. Honza > > Signed-off-by: Jeff Mahoney > --- > fs/reiserfs/inode.c | 38 ++++++++++++++++++++------------------ > fs/reiserfs/super.c | 48 +++++++++++++++++++++++------------------------- > fs/reiserfs/xattr.c | 46 +++++++++++++++++----------------------------- > fs/reiserfs/xattr_acl.c | 16 ++++++++++------ > 4 files changed, 70 insertions(+), 78 deletions(-) > > --- a/fs/reiserfs/inode.c 2013-08-07 10:23:30.681636170 -0400 > +++ b/fs/reiserfs/inode.c 2013-08-07 10:23:31.613622029 -0400 > @@ -30,7 +30,6 @@ void reiserfs_evict_inode(struct inode * > JOURNAL_PER_BALANCE_CNT * 2 + > 2 * REISERFS_QUOTA_INIT_BLOCKS(inode->i_sb); > struct reiserfs_transaction_handle th; > - int depth; > int err; > > if (!inode->i_nlink && !is_bad_inode(inode)) > @@ -40,12 +39,14 @@ void reiserfs_evict_inode(struct inode * > if (inode->i_nlink) > goto no_delete; > > - depth = reiserfs_write_lock_once(inode->i_sb); > - > /* The = 0 happens when we abort creating a new inode for some reason like lack of space.. */ > if (!(inode->i_state & I_NEW) && INODE_PKEY(inode)->k_objectid != 0) { /* also handles bad_inode case */ > + int depth; > + > reiserfs_delete_xattrs(inode); > > + depth = reiserfs_write_lock_once(inode->i_sb); > + > if (journal_begin(&th, inode->i_sb, jbegin_count)) > goto out; > reiserfs_update_inode_transaction(inode); > @@ -72,12 +73,12 @@ void reiserfs_evict_inode(struct inode * > /* all items of file are deleted, so we can remove "save" link */ > remove_save_link(inode, 0 /* not truncate */ ); /* we can't do anything > * about an error here */ > +out: > + reiserfs_write_unlock_once(inode->i_sb, depth); > } else { > /* no object items are in the tree */ > ; > } > - out: > - reiserfs_write_unlock_once(inode->i_sb, depth); > clear_inode(inode); /* note this must go after the journal_end to prevent deadlock */ > dquot_drop(inode); > inode->i_blocks = 0; > @@ -1941,7 +1942,9 @@ int reiserfs_new_inode(struct reiserfs_t > } > > if (reiserfs_posixacl(inode->i_sb)) { > + reiserfs_write_unlock(inode->i_sb); > retval = reiserfs_inherit_default_acl(th, dir, dentry, inode); > + reiserfs_write_lock(inode->i_sb); > if (retval) { > err = retval; > reiserfs_check_path(&path_to_key); > @@ -1956,7 +1959,9 @@ int reiserfs_new_inode(struct reiserfs_t > inode->i_flags |= S_PRIVATE; > > if (security->name) { > + reiserfs_write_unlock(inode->i_sb); > retval = reiserfs_security_write(th, inode, security); > + reiserfs_write_lock(inode->i_sb); > if (retval) { > err = retval; > reiserfs_check_path(&path_to_key); > @@ -3129,6 +3134,7 @@ int reiserfs_setattr(struct dentry *dent > */ > if (get_inode_item_key_version(inode) == KEY_FORMAT_3_5 && > attr->ia_size > MAX_NON_LFS) { > + reiserfs_write_unlock_once(inode->i_sb, depth); > error = -EFBIG; > goto out; > } > @@ -3150,8 +3156,10 @@ int reiserfs_setattr(struct dentry *dent > if (err) > error = err; > } > - if (error) > + if (error) { > + reiserfs_write_unlock_once(inode->i_sb, depth); > goto out; > + } > /* > * file size is changed, ctime and mtime are > * to be updated > @@ -3159,6 +3167,7 @@ int reiserfs_setattr(struct dentry *dent > attr->ia_valid |= (ATTR_MTIME | ATTR_CTIME); > } > } > + reiserfs_write_unlock_once(inode->i_sb, depth); > > if ((((attr->ia_valid & ATTR_UID) && (from_kuid(&init_user_ns, attr->ia_uid) & ~0xffff)) || > ((attr->ia_valid & ATTR_GID) && (from_kgid(&init_user_ns, attr->ia_gid) & ~0xffff))) && > @@ -3183,14 +3192,16 @@ int reiserfs_setattr(struct dentry *dent > return error; > > /* (user+group)*(old+new) structure - we count quota info and , inode write (sb, inode) */ > + depth = reiserfs_write_lock_once(inode->i_sb); > error = journal_begin(&th, inode->i_sb, jbegin_count); > + reiserfs_write_unlock_once(inode->i_sb, depth); > if (error) > goto out; > - reiserfs_write_unlock_once(inode->i_sb, depth); > error = dquot_transfer(inode, attr); > depth = reiserfs_write_lock_once(inode->i_sb); > if (error) { > journal_end(&th, inode->i_sb, jbegin_count); > + reiserfs_write_unlock_once(inode->i_sb, depth); > goto out; > } > > @@ -3202,17 +3213,11 @@ int reiserfs_setattr(struct dentry *dent > inode->i_gid = attr->ia_gid; > mark_inode_dirty(inode); > error = journal_end(&th, inode->i_sb, jbegin_count); > + reiserfs_write_unlock_once(inode->i_sb, depth); > if (error) > goto out; > } > > - /* > - * Relax the lock here, as it might truncate the > - * inode pages and wait for inode pages locks. > - * To release such page lock, the owner needs the > - * reiserfs lock > - */ > - reiserfs_write_unlock_once(inode->i_sb, depth); > if ((attr->ia_valid & ATTR_SIZE) && > attr->ia_size != i_size_read(inode)) { > error = inode_newsize_ok(inode, attr->ia_size); > @@ -3226,16 +3231,13 @@ int reiserfs_setattr(struct dentry *dent > setattr_copy(inode, attr); > mark_inode_dirty(inode); > } > - depth = reiserfs_write_lock_once(inode->i_sb); > > if (!error && reiserfs_posixacl(inode->i_sb)) { > if (attr->ia_valid & ATTR_MODE) > error = reiserfs_acl_chmod(inode); > } > > - out: > - reiserfs_write_unlock_once(inode->i_sb, depth); > - > +out: > return error; > } > > --- a/fs/reiserfs/super.c 2013-08-07 10:23:30.681636170 -0400 > +++ b/fs/reiserfs/super.c 2013-08-07 10:23:31.637621664 -0400 > @@ -1335,7 +1335,7 @@ static int reiserfs_remount(struct super > kfree(qf_names[i]); > #endif > err = -EINVAL; > - goto out_unlock; > + goto out_err_unlock; > } > #ifdef CONFIG_QUOTA > handle_quota_files(s, qf_names, &qfmt); > @@ -1379,35 +1379,32 @@ static int reiserfs_remount(struct super > if (blocks) { > err = reiserfs_resize(s, blocks); > if (err != 0) > - goto out_unlock; > + goto out_err_unlock; > } > > if (*mount_flags & MS_RDONLY) { > + reiserfs_write_unlock(s); > reiserfs_xattr_init(s, *mount_flags); > /* remount read-only */ > if (s->s_flags & MS_RDONLY) > /* it is read-only already */ > - goto out_ok; > + goto out_ok_unlocked; > > - /* > - * Drop write lock. Quota will retake it when needed and lock > - * ordering requires calling dquot_suspend() without it. > - */ > - reiserfs_write_unlock(s); > err = dquot_suspend(s, -1); > if (err < 0) > goto out_err; > - reiserfs_write_lock(s); > > /* try to remount file system with read-only permissions */ > if (sb_umount_state(rs) == REISERFS_VALID_FS > || REISERFS_SB(s)->s_mount_state != REISERFS_VALID_FS) { > - goto out_ok; > + goto out_ok_unlocked; > } > > + reiserfs_write_lock(s); > + > err = journal_begin(&th, s, 10); > if (err) > - goto out_unlock; > + goto out_err_unlock; > > /* Mounting a rw partition read-only. */ > reiserfs_prepare_for_journal(s, SB_BUFFER_WITH_SB(s), 1); > @@ -1416,13 +1413,14 @@ static int reiserfs_remount(struct super > } else { > /* remount read-write */ > if (!(s->s_flags & MS_RDONLY)) { > + reiserfs_write_unlock(s); > reiserfs_xattr_init(s, *mount_flags); > - goto out_ok; /* We are read-write already */ > + goto out_ok_unlocked; /* We are read-write already */ > } > > if (reiserfs_is_journal_aborted(journal)) { > err = journal->j_errno; > - goto out_unlock; > + goto out_err_unlock; > } > > handle_data_mode(s, mount_options); > @@ -1431,7 +1429,7 @@ static int reiserfs_remount(struct super > s->s_flags &= ~MS_RDONLY; /* now it is safe to call journal_begin */ > err = journal_begin(&th, s, 10); > if (err) > - goto out_unlock; > + goto out_err_unlock; > > /* Mount a partition which is read-only, read-write */ > reiserfs_prepare_for_journal(s, SB_BUFFER_WITH_SB(s), 1); > @@ -1448,26 +1446,22 @@ static int reiserfs_remount(struct super > SB_JOURNAL(s)->j_must_wait = 1; > err = journal_end(&th, s, 10); > if (err) > - goto out_unlock; > + goto out_err_unlock; > > + reiserfs_write_unlock(s); > if (!(*mount_flags & MS_RDONLY)) { > - /* > - * Drop write lock. Quota will retake it when needed and lock > - * ordering requires calling dquot_resume() without it. > - */ > - reiserfs_write_unlock(s); > dquot_resume(s, -1); > reiserfs_write_lock(s); > finish_unfinished(s); > + reiserfs_write_unlock(s); > reiserfs_xattr_init(s, *mount_flags); > } > > -out_ok: > +out_ok_unlocked: > replace_mount_options(s, new_opts); > - reiserfs_write_unlock(s); > return 0; > > -out_unlock: > +out_err_unlock: > reiserfs_write_unlock(s); > out_err: > kfree(new_opts); > @@ -2014,12 +2008,14 @@ static int reiserfs_fill_super(struct su > goto error; > } > > + reiserfs_write_unlock(s); > if ((errval = reiserfs_lookup_privroot(s)) || > (errval = reiserfs_xattr_init(s, s->s_flags))) { > dput(s->s_root); > s->s_root = NULL; > - goto error; > + goto error_unlocked; > } > + reiserfs_write_lock(s); > > /* look for files which were to be removed in previous session */ > finish_unfinished(s); > @@ -2028,12 +2024,14 @@ static int reiserfs_fill_super(struct su > reiserfs_info(s, "using 3.5.x disk format\n"); > } > > + reiserfs_write_unlock(s); > if ((errval = reiserfs_lookup_privroot(s)) || > (errval = reiserfs_xattr_init(s, s->s_flags))) { > dput(s->s_root); > s->s_root = NULL; > - goto error; > + goto error_unlocked; > } > + reiserfs_write_lock(s); > } > // mark hash in super block: it could be unset. overwrite should be ok > set_sb_hash_function_code(rs, function2code(sbi->s_hash_function)); > --- a/fs/reiserfs/xattr.c 2013-08-07 10:23:30.681636170 -0400 > +++ b/fs/reiserfs/xattr.c 2013-08-07 10:23:31.649621482 -0400 > @@ -81,8 +81,7 @@ static int xattr_unlink(struct inode *di > int error; > BUG_ON(!mutex_is_locked(&dir->i_mutex)); > > - reiserfs_mutex_lock_nested_safe(&dentry->d_inode->i_mutex, > - I_MUTEX_CHILD, dir->i_sb); > + mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_CHILD); > error = dir->i_op->unlink(dir, dentry); > mutex_unlock(&dentry->d_inode->i_mutex); > > @@ -96,8 +95,7 @@ static int xattr_rmdir(struct inode *dir > int error; > BUG_ON(!mutex_is_locked(&dir->i_mutex)); > > - reiserfs_mutex_lock_nested_safe(&dentry->d_inode->i_mutex, > - I_MUTEX_CHILD, dir->i_sb); > + mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_CHILD); > error = dir->i_op->rmdir(dir, dentry); > if (!error) > dentry->d_inode->i_flags |= S_DEAD; > @@ -232,22 +230,17 @@ static int reiserfs_for_each_xattr(struc > if (IS_PRIVATE(inode) || get_inode_sd_version(inode) == STAT_DATA_V1) > return 0; > > - reiserfs_write_unlock(inode->i_sb); > dir = open_xa_dir(inode, XATTR_REPLACE); > if (IS_ERR(dir)) { > err = PTR_ERR(dir); > - reiserfs_write_lock(inode->i_sb); > goto out; > } else if (!dir->d_inode) { > err = 0; > - reiserfs_write_lock(inode->i_sb); > goto out_dir; > } > > mutex_lock_nested(&dir->d_inode->i_mutex, I_MUTEX_XATTR); > > - reiserfs_write_lock(inode->i_sb); > - > buf.xadir = dir; > while (1) { > err = reiserfs_readdir_inode(dir->d_inode, &buf.ctx); > @@ -281,14 +274,17 @@ static int reiserfs_for_each_xattr(struc > int blocks = JOURNAL_PER_BALANCE_CNT * 2 + 2 + > 4 * REISERFS_QUOTA_TRANS_BLOCKS(inode->i_sb); > struct reiserfs_transaction_handle th; > + reiserfs_write_lock(inode->i_sb); > err = journal_begin(&th, inode->i_sb, blocks); > + reiserfs_write_unlock(inode->i_sb); > if (!err) { > int jerror; > - reiserfs_mutex_lock_nested_safe( > - &dir->d_parent->d_inode->i_mutex, > - I_MUTEX_XATTR, inode->i_sb); > + mutex_lock_nested(&dir->d_parent->d_inode->i_mutex, > + I_MUTEX_XATTR); > err = action(dir, data); > + reiserfs_write_lock(inode->i_sb); > jerror = journal_end(&th, inode->i_sb, blocks); > + reiserfs_write_unlock(inode->i_sb); > mutex_unlock(&dir->d_parent->d_inode->i_mutex); > err = jerror ?: err; > } > @@ -455,9 +451,7 @@ static int lookup_and_delete_xattr(struc > } > > if (dentry->d_inode) { > - reiserfs_write_lock(inode->i_sb); > err = xattr_unlink(xadir->d_inode, dentry); > - reiserfs_write_unlock(inode->i_sb); > update_ctime(inode); > } > > @@ -491,24 +485,17 @@ reiserfs_xattr_set_handle(struct reiserf > if (get_inode_sd_version(inode) == STAT_DATA_V1) > return -EOPNOTSUPP; > > - reiserfs_write_unlock(inode->i_sb); > - > if (!buffer) { > err = lookup_and_delete_xattr(inode, name); > - reiserfs_write_lock(inode->i_sb); > return err; > } > > dentry = xattr_lookup(inode, name, flags); > - if (IS_ERR(dentry)) { > - reiserfs_write_lock(inode->i_sb); > + if (IS_ERR(dentry)) > return PTR_ERR(dentry); > - } > > down_write(&REISERFS_I(inode)->i_xattr_sem); > > - reiserfs_write_lock(inode->i_sb); > - > xahash = xattr_hash(buffer, buffer_size); > while (buffer_pos < buffer_size || buffer_pos == 0) { > size_t chunk; > @@ -538,6 +525,7 @@ reiserfs_xattr_set_handle(struct reiserf > rxh->h_hash = cpu_to_le32(xahash); > } > > + reiserfs_write_lock(inode->i_sb); > err = __reiserfs_write_begin(page, page_offset, chunk + skip); > if (!err) { > if (buffer) > @@ -546,6 +534,7 @@ reiserfs_xattr_set_handle(struct reiserf > page_offset + chunk + > skip); > } > + reiserfs_write_unlock(inode->i_sb); > unlock_page(page); > reiserfs_put_page(page); > buffer_pos += chunk; > @@ -563,10 +552,8 @@ reiserfs_xattr_set_handle(struct reiserf > .ia_valid = ATTR_SIZE | ATTR_CTIME, > }; > > - reiserfs_write_unlock(inode->i_sb); > mutex_lock_nested(&dentry->d_inode->i_mutex, I_MUTEX_XATTR); > inode_dio_wait(dentry->d_inode); > - reiserfs_write_lock(inode->i_sb); > > err = reiserfs_setattr(dentry, &newattrs); > mutex_unlock(&dentry->d_inode->i_mutex); > @@ -592,18 +579,19 @@ int reiserfs_xattr_set(struct inode *ino > > reiserfs_write_lock(inode->i_sb); > error = journal_begin(&th, inode->i_sb, jbegin_count); > + reiserfs_write_unlock(inode->i_sb); > if (error) { > - reiserfs_write_unlock(inode->i_sb); > return error; > } > > error = reiserfs_xattr_set_handle(&th, inode, name, > buffer, buffer_size, flags); > > + reiserfs_write_lock(inode->i_sb); > error2 = journal_end(&th, inode->i_sb, jbegin_count); > + reiserfs_write_unlock(inode->i_sb); > if (error == 0) > error = error2; > - reiserfs_write_unlock(inode->i_sb); > > return error; > } > @@ -968,7 +956,7 @@ int reiserfs_lookup_privroot(struct supe > int err = 0; > > /* If we don't have the privroot located yet - go find it */ > - reiserfs_mutex_lock_safe(&s->s_root->d_inode->i_mutex, s); > + mutex_lock(&s->s_root->d_inode->i_mutex); > dentry = lookup_one_len(PRIVROOT_NAME, s->s_root, > strlen(PRIVROOT_NAME)); > if (!IS_ERR(dentry)) { > @@ -996,14 +984,14 @@ int reiserfs_xattr_init(struct super_blo > goto error; > > if (!privroot->d_inode && !(mount_flags & MS_RDONLY)) { > - reiserfs_mutex_lock_safe(&s->s_root->d_inode->i_mutex, s); > + mutex_lock(&s->s_root->d_inode->i_mutex); > err = create_privroot(REISERFS_SB(s)->priv_root); > mutex_unlock(&s->s_root->d_inode->i_mutex); > } > > if (privroot->d_inode) { > s->s_xattr = reiserfs_xattr_handlers; > - reiserfs_mutex_lock_safe(&privroot->d_inode->i_mutex, s); > + mutex_lock(&privroot->d_inode->i_mutex); > if (!REISERFS_SB(s)->xattr_root) { > struct dentry *dentry; > dentry = lookup_one_len(XAROOT_NAME, privroot, > --- a/fs/reiserfs/xattr_acl.c 2013-08-07 10:23:30.681636170 -0400 > +++ b/fs/reiserfs/xattr_acl.c 2013-08-07 10:23:31.661621300 -0400 > @@ -49,13 +49,15 @@ posix_acl_set(struct dentry *dentry, con > > reiserfs_write_lock(inode->i_sb); > error = journal_begin(&th, inode->i_sb, jcreate_blocks); > + reiserfs_write_unlock(inode->i_sb); > if (error == 0) { > error = reiserfs_set_acl(&th, inode, type, acl); > + reiserfs_write_lock(inode->i_sb); > error2 = journal_end(&th, inode->i_sb, jcreate_blocks); > + reiserfs_write_unlock(inode->i_sb); > if (error2) > error = error2; > } > - reiserfs_write_unlock(inode->i_sb); > > release_and_out: > posix_acl_release(acl); > @@ -435,12 +437,14 @@ int reiserfs_cache_default_acl(struct in > return nblocks; > } > > +/* > + * Called under i_mutex > + */ > int reiserfs_acl_chmod(struct inode *inode) > { > struct reiserfs_transaction_handle th; > struct posix_acl *acl; > size_t size; > - int depth; > int error; > > if (IS_PRIVATE(inode)) > @@ -454,9 +458,7 @@ int reiserfs_acl_chmod(struct inode *ino > return 0; > } > > - reiserfs_write_unlock(inode->i_sb); > acl = reiserfs_get_acl(inode, ACL_TYPE_ACCESS); > - reiserfs_write_lock(inode->i_sb); > if (!acl) > return 0; > if (IS_ERR(acl)) > @@ -466,16 +468,18 @@ int reiserfs_acl_chmod(struct inode *ino > return error; > > size = reiserfs_xattr_nblocks(inode, reiserfs_acl_size(acl->a_count)); > - depth = reiserfs_write_lock_once(inode->i_sb); > + reiserfs_write_lock(inode->i_sb); > error = journal_begin(&th, inode->i_sb, size * 2); > + reiserfs_write_unlock(inode->i_sb); > if (!error) { > int error2; > error = reiserfs_set_acl(&th, inode, ACL_TYPE_ACCESS, acl); > + reiserfs_write_lock(inode->i_sb); > error2 = journal_end(&th, inode->i_sb, size * 2); > + reiserfs_write_unlock(inode->i_sb); > if (error2) > error = error2; > } > - reiserfs_write_unlock_once(inode->i_sb, depth); > posix_acl_release(acl); > return error; > } > > -- Jan Kara SUSE Labs, CR