Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751440AbcDPDCW (ORCPT ); Fri, 15 Apr 2016 23:02:22 -0400 Received: from mail-io0-f172.google.com ([209.85.223.172]:34651 "EHLO mail-io0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751069AbcDPDCT (ORCPT ); Fri, 15 Apr 2016 23:02:19 -0400 Subject: Re: [PATCH 15/15] parallel lookups: actual switch to rwsem Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Content-Type: multipart/signed; boundary="Apple-Mail=_9854266E-3CF3-490D-8114-D35EBDC8E744"; protocol="application/pgp-signature"; micalg=pgp-sha256 X-Pgp-Agent: GPGMail 2.6b2 From: Andreas Dilger In-Reply-To: <1460768127-31822-15-git-send-email-viro@ZenIV.linux.org.uk> Date: Fri, 15 Apr 2016 21:02:06 -0600 Cc: Linus Torvalds , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org Message-Id: References: <20160416005232.GV25498@ZenIV.linux.org.uk> <1460768127-31822-15-git-send-email-viro@ZenIV.linux.org.uk> To: Al Viro X-Mailer: Apple Mail (2.3124) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11064 Lines: 353 --Apple-Mail=_9854266E-3CF3-490D-8114-D35EBDC8E744 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii On Apr 15, 2016, at 6:55 PM, Al Viro wrote: >=20 > From: Al Viro >=20 > ta-da! >=20 > The main issue is the lack of down_write_killable(), so the places > like readdir.c switched to plain inode_lock(); once killable > variants of rwsem primitives appear, that'll be dealt with. >=20 > lockdep side also might need more work >=20 > Signed-off-by: Al Viro > --- > fs/btrfs/ioctl.c | 16 ++++++++++------ > fs/configfs/inode.c | 2 +- > fs/dcache.c | 9 +++++---- > fs/gfs2/ops_fstype.c | 2 +- > fs/inode.c | 12 ++++++------ > fs/namei.c | 4 ++-- > fs/ocfs2/inode.c | 2 +- > fs/overlayfs/readdir.c | 4 +++- > fs/readdir.c | 7 ++++--- > include/linux/fs.h | 12 ++++++------ > 10 files changed, 39 insertions(+), 31 deletions(-) >=20 > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 053e677..db1e830 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -837,9 +837,11 @@ static noinline int btrfs_mksubvol(struct path = *parent, > struct dentry *dentry; > int error; >=20 > - error =3D mutex_lock_killable_nested(&dir->i_mutex, = I_MUTEX_PARENT); > - if (error =3D=3D -EINTR) > - return error; > + inode_lock_nested(dir, I_MUTEX_PARENT); > + // XXX: should've been > + // mutex_lock_killable_nested(&dir->i_mutex, I_MUTEX_PARENT); > + // if (error =3D=3D -EINTR) > + // return error; >=20 > dentry =3D lookup_one_len(name, parent->dentry, namelen); > error =3D PTR_ERR(dentry); > @@ -2366,9 +2368,11 @@ static noinline int = btrfs_ioctl_snap_destroy(struct file *file, > goto out; >=20 >=20 > - err =3D mutex_lock_killable_nested(&dir->i_mutex, = I_MUTEX_PARENT); > - if (err =3D=3D -EINTR) > - goto out_drop_write; > + inode_lock_nested(dir, I_MUTEX_PARENT); > + // XXX: should've been > + // err =3D mutex_lock_killable_nested(&dir->i_mutex, = I_MUTEX_PARENT); > + // if (err =3D=3D -EINTR) > + // goto out_drop_write; > dentry =3D lookup_one_len(vol_args->name, parent, namelen); > if (IS_ERR(dentry)) { > err =3D PTR_ERR(dentry); > diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c > index 03d124a..0387968 100644 > --- a/fs/configfs/inode.c > +++ b/fs/configfs/inode.c > @@ -156,7 +156,7 @@ static void configfs_set_inode_lock_class(struct = configfs_dirent *sd, >=20 > if (depth > 0) { > if (depth <=3D ARRAY_SIZE(default_group_class)) { > - lockdep_set_class(&inode->i_mutex, > + lockdep_set_class(&inode->i_rwsem, > &default_group_class[depth - = 1]); > } else { > /* > diff --git a/fs/dcache.c b/fs/dcache.c > index 5965588..d110040 100644 > --- a/fs/dcache.c > +++ b/fs/dcache.c > @@ -2911,7 +2911,8 @@ struct dentry *d_ancestor(struct dentry *p1, = struct dentry *p2) > static int __d_unalias(struct inode *inode, > struct dentry *dentry, struct dentry *alias) > { > - struct mutex *m1 =3D NULL, *m2 =3D NULL; > + struct mutex *m1 =3D NULL; > + struct rw_semaphore *m2 =3D NULL; > int ret =3D -ESTALE; >=20 > /* If alias and dentry share a parent, then no extra locks = required */ > @@ -2922,15 +2923,15 @@ static int __d_unalias(struct inode *inode, > if (!mutex_trylock(&dentry->d_sb->s_vfs_rename_mutex)) > goto out_err; > m1 =3D &dentry->d_sb->s_vfs_rename_mutex; > - if (!inode_trylock(alias->d_parent->d_inode)) > + if (!down_read_trylock(&alias->d_parent->d_inode->i_rwsem)) > goto out_err; > - m2 =3D &alias->d_parent->d_inode->i_mutex; > + m2 =3D &alias->d_parent->d_inode->i_rwsem; > out_unalias: > __d_move(alias, dentry, false); > ret =3D 0; > out_err: > if (m2) > - mutex_unlock(m2); > + up_read(m2); > if (m1) > mutex_unlock(m1); > return ret; > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c > index c09c63d..4546360 100644 > --- a/fs/gfs2/ops_fstype.c > +++ b/fs/gfs2/ops_fstype.c > @@ -824,7 +824,7 @@ static int init_inodes(struct gfs2_sbd *sdp, int = undo) > * i_mutex on quota files is special. Since this inode is hidden = system > * file, we are safe to define locking ourselves. > */ > - lockdep_set_class(&sdp->sd_quota_inode->i_mutex, > + lockdep_set_class(&sdp->sd_quota_inode->i_rwsem, > &gfs2_quota_imutex_key); >=20 > error =3D gfs2_rindex_update(sdp); > diff --git a/fs/inode.c b/fs/inode.c > index 4b884f7..4ccbc21 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -166,8 +166,8 @@ int inode_init_always(struct super_block *sb, = struct inode *inode) > spin_lock_init(&inode->i_lock); > lockdep_set_class(&inode->i_lock, &sb->s_type->i_lock_key); >=20 > - mutex_init(&inode->i_mutex); > - lockdep_set_class(&inode->i_mutex, &sb->s_type->i_mutex_key); > + init_rwsem(&inode->i_rwsem); > + lockdep_set_class(&inode->i_rwsem, &sb->s_type->i_mutex_key); >=20 > atomic_set(&inode->i_dio_count, 0); >=20 > @@ -925,13 +925,13 @@ void lockdep_annotate_inode_mutex_key(struct = inode *inode) > struct file_system_type *type =3D inode->i_sb->s_type; >=20 > /* Set new key only if filesystem hasn't already changed = it */ > - if (lockdep_match_class(&inode->i_mutex, = &type->i_mutex_key)) { > + if (lockdep_match_class(&inode->i_rwsem, = &type->i_mutex_key)) { > /* > * ensure nobody is actually holding i_mutex > */ > - mutex_destroy(&inode->i_mutex); > - mutex_init(&inode->i_mutex); > - lockdep_set_class(&inode->i_mutex, > + // mutex_destroy(&inode->i_mutex); > + init_rwsem(&inode->i_rwsem); > + lockdep_set_class(&inode->i_rwsem, > &type->i_mutex_dir_key); > } > } > diff --git a/fs/namei.c b/fs/namei.c > index eb879d6..877e9ef 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -1607,7 +1607,7 @@ static struct dentry *lookup_slow(const struct = qstr *name, > struct inode *inode =3D dir->d_inode; > DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq); >=20 > - inode_lock(inode); > + down_read(&inode->i_rwsem); Wouldn't it make sense to have helpers like "inode_read_lock(inode)" or = similar, so that it is consistent with other parts of the code and easier to = find? It's a bit strange to have the filesystems use "inode_lock()" and some = places here use "inode_lock_nested()", but other places use up_read() and = down_read() directly on &inode->i_rwsem. That would also simplify delegating the = directory locking to the filesystems in the future. Cheers, Andreas > /* Don't go there if it's already dead */ > if (unlikely(IS_DEADDIR(inode))) > goto out; > @@ -1638,7 +1638,7 @@ again: > } > } > out: > - inode_unlock(inode); > + up_read(&inode->i_rwsem); > return dentry; > } >=20 > diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c > index 12f4a9e..0748777 100644 > --- a/fs/ocfs2/inode.c > +++ b/fs/ocfs2/inode.c > @@ -262,7 +262,7 @@ static int ocfs2_init_locked_inode(struct inode = *inode, void *opaque) > inode->i_ino =3D args->fi_ino; > OCFS2_I(inode)->ip_blkno =3D args->fi_blkno; > if (args->fi_sysfile_type !=3D 0) > - lockdep_set_class(&inode->i_mutex, > + lockdep_set_class(&inode->i_rwsem, > &ocfs2_sysfile_lock_key[args->fi_sysfile_type]); > if (args->fi_sysfile_type =3D=3D USER_QUOTA_SYSTEM_INODE || > args->fi_sysfile_type =3D=3D GROUP_QUOTA_SYSTEM_INODE || > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c > index 6ec1e43..da186ee 100644 > --- a/fs/overlayfs/readdir.c > +++ b/fs/overlayfs/readdir.c > @@ -218,7 +218,9 @@ static int ovl_check_whiteouts(struct dentry *dir, = struct ovl_readdir_data *rdd) > cap_raise(override_cred->cap_effective, CAP_DAC_OVERRIDE); > old_cred =3D override_creds(override_cred); >=20 > - err =3D mutex_lock_killable(&dir->d_inode->i_mutex); > + inode_lock(dir->d_inode); > + err =3D 0; > + // XXX: err =3D mutex_lock_killable(&dir->d_inode->i_mutex); > if (!err) { > while (rdd->first_maybe_whiteout) { > p =3D rdd->first_maybe_whiteout; > diff --git a/fs/readdir.c b/fs/readdir.c > index e69ef3b..bf583e8 100644 > --- a/fs/readdir.c > +++ b/fs/readdir.c > @@ -32,9 +32,10 @@ int iterate_dir(struct file *file, struct = dir_context *ctx) > if (res) > goto out; >=20 > - res =3D mutex_lock_killable(&inode->i_mutex); > - if (res) > - goto out; > + inode_lock(inode); > + // res =3D mutex_lock_killable(&inode->i_mutex); > + // if (res) > + // goto out; >=20 > res =3D -ENOENT; > if (!IS_DEADDIR(inode)) { > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 0a32045..313ad28 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -647,7 +647,7 @@ struct inode { >=20 > /* Misc */ > unsigned long i_state; > - struct mutex i_mutex; > + struct rw_semaphore i_rwsem; >=20 > unsigned long dirtied_when; /* jiffies of first = dirtying */ > unsigned long dirtied_time_when; > @@ -734,27 +734,27 @@ enum inode_i_mutex_lock_class >=20 > static inline void inode_lock(struct inode *inode) > { > - mutex_lock(&inode->i_mutex); > + down_write(&inode->i_rwsem); > } >=20 > static inline void inode_unlock(struct inode *inode) > { > - mutex_unlock(&inode->i_mutex); > + up_write(&inode->i_rwsem); > } >=20 > static inline int inode_trylock(struct inode *inode) > { > - return mutex_trylock(&inode->i_mutex); > + return down_write_trylock(&inode->i_rwsem); > } >=20 > static inline int inode_is_locked(struct inode *inode) > { > - return mutex_is_locked(&inode->i_mutex); > + return rwsem_is_locked(&inode->i_rwsem); > } >=20 > static inline void inode_lock_nested(struct inode *inode, unsigned = subclass) > { > - mutex_lock_nested(&inode->i_mutex, subclass); > + down_write_nested(&inode->i_rwsem, subclass); > } >=20 > void lock_two_nondirectories(struct inode *, struct inode*); > -- > 2.8.0.rc3 >=20 > -- > To unsubscribe from this list: send the line "unsubscribe = linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Andreas --Apple-Mail=_9854266E-3CF3-490D-8114-D35EBDC8E744 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP using GPGMail -----BEGIN PGP SIGNATURE----- Comment: GPGTools - http://gpgtools.org iQIVAwUBVxGrLnKl2rkXzB/gAQiHiw//bziOWg0sJiJlKn/CrU/01Hs9j0Qe8JmW milp3i7t7dNp/94b3FCbzL0OX4Z+tPyTz+8XVhd+QRpwBwnsTe/3ipN5aZZy8aVN dkWndUiYt+wgNOrRQFiXVuPssBbsv4GTU642lBTW3FOLdoLPNX8GPxjmYBxEZJrO ICDf+kweSkpc7J740LjRPSMqHrOnfk+VFXneVw3lnNoPu5cHLXfEalmbBNfQowmY 3g10Usipb39lstuTnklD7NdazzSKQrwSVVxCt0QKON/WmQJYASYMservrG3/7UiU eU3l5KqMyV9Csrj6qkFgTtgzaMK6zwvYajl9qiezJYTENgySYcVCfcs6Vjqy+erP tHgFqKQod29qxqJTfiMh8Q86c0igfzi8/YjU5xLwzKv8fOTYE9Z7SwV5WNI2eBIU k2OBmqZCenLT6ERtqVbYy2P5t/VtrUGmYNtonvZ888onuy++Ru6UdKHRcuCR9FCO TQ5lGUxKYVEy7VD38wVrxqmQugnYSHV3bSYavXYutCKa+3TLUwV5jjIgRkahy0Lf IFDDmEWV2VA3a9xulpBC+2ucZz6yHdudp3Jxh334q2bPCU8z4cBOK4V91jEM9eFX IExf9a+8dmpewsSF2IYDsKisD95qgNPonePNl0hBfAmBGB46Qp7u8z77ACGv1vJG KaA0KHOstZg= =k6nK -----END PGP SIGNATURE----- --Apple-Mail=_9854266E-3CF3-490D-8114-D35EBDC8E744--