| Message ID | 20170816154127.7048-13-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Aug 16, 2017, at 9:41 AM, Jan Kara <jack@suse.cz> wrote: > > Push down acquisition of dqio_sem into ->get_next_id() callback. Mostly > for consistency with other operations. > > Signed-off-by: Jan Kara <jack@suse.cz> I was initially thinking that this patch also needed to modify the filesystem-specific code in a similar way, but ext4 and reiserfs use dquot_get_next_id() internally, ocfs2 appears to have its own locking, and XFS is doing completely its own thing. It is a bit confusing that there are both dquot_operation.get_next_id as well as quota_format_ops.get_next_id methods that are similar but not totally the same thing. It might make sense to rename the quota_format_ops method like .get_next_id_format or similar to be able to distinguish them? Patch itself is fine though. Reviewed-by: Andreas Dilger <adilger@dilger.ca> > --- > fs/quota/dquot.c | 6 +----- > fs/quota/quota_v2.c | 8 +++++++- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 3b3c7f094ff8..332f7026edad 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -2067,16 +2067,12 @@ EXPORT_SYMBOL(dquot_commit_info); > int dquot_get_next_id(struct super_block *sb, struct kqid *qid) > { > struct quota_info *dqopt = sb_dqopt(sb); > - int err; > > if (!sb_has_quota_active(sb, qid->type)) > return -ESRCH; > if (!dqopt->ops[qid->type]->get_next_id) > return -ENOSYS; > - down_read(&dqopt->dqio_sem); > - err = dqopt->ops[qid->type]->get_next_id(sb, qid); > - up_read(&dqopt->dqio_sem); > - return err; > + return dqopt->ops[qid->type]->get_next_id(sb, qid); > } > EXPORT_SYMBOL(dquot_get_next_id); > > diff --git a/fs/quota/quota_v2.c b/fs/quota/quota_v2.c > index 8f54b159e423..f82638e43c07 100644 > --- a/fs/quota/quota_v2.c > +++ b/fs/quota/quota_v2.c > @@ -340,7 +340,13 @@ static int v2_free_file_info(struct super_block *sb, int type) > > static int v2_get_next_id(struct super_block *sb, struct kqid *qid) > { > - return qtree_get_next_id(sb_dqinfo(sb, qid->type)->dqi_priv, qid); > + struct quota_info *dqopt = sb_dqopt(sb); > + int ret; > + > + down_read(&dqopt->dqio_sem); > + ret = qtree_get_next_id(sb_dqinfo(sb, qid->type)->dqi_priv, qid); > + up_read(&dqopt->dqio_sem); > + return ret; > } > > static const struct quota_format_ops v2_format_ops = { > -- > 2.12.3 > Cheers, Andreas
On Wed 16-08-17 11:08:12, Andreas Dilger wrote: > On Aug 16, 2017, at 9:41 AM, Jan Kara <jack@suse.cz> wrote: > > > > Push down acquisition of dqio_sem into ->get_next_id() callback. Mostly > > for consistency with other operations. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > I was initially thinking that this patch also needed to modify the > filesystem-specific code in a similar way, but ext4 and reiserfs > use dquot_get_next_id() internally, ocfs2 appears to have its own > locking, and XFS is doing completely its own thing. > > It is a bit confusing that there are both dquot_operation.get_next_id > as well as quota_format_ops.get_next_id methods that are similar but > not totally the same thing. It might make sense to rename the > quota_format_ops method like .get_next_id_format or similar to be able > to distinguish them? Yeah, they are two different levels of abstraction of the same operation. I agree that the fact that names are matching is a bit confusing. I'll think how to differentiate these names... Honza
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 3b3c7f094ff8..332f7026edad 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -2067,16 +2067,12 @@ EXPORT_SYMBOL(dquot_commit_info); int dquot_get_next_id(struct super_block *sb, struct kqid *qid) { struct quota_info *dqopt = sb_dqopt(sb); - int err; if (!sb_has_quota_active(sb, qid->type)) return -ESRCH; if (!dqopt->ops[qid->type]->get_next_id) return -ENOSYS; - down_read(&dqopt->dqio_sem); - err = dqopt->ops[qid->type]->get_next_id(sb, qid); - up_read(&dqopt->dqio_sem); - return err; + return dqopt->ops[qid->type]->get_next_id(sb, qid); } EXPORT_SYMBOL(dquot_get_next_id); diff --git a/fs/quota/quota_v2.c b/fs/quota/quota_v2.c index 8f54b159e423..f82638e43c07 100644 --- a/fs/quota/quota_v2.c +++ b/fs/quota/quota_v2.c @@ -340,7 +340,13 @@ static int v2_free_file_info(struct super_block *sb, int type) static int v2_get_next_id(struct super_block *sb, struct kqid *qid) { - return qtree_get_next_id(sb_dqinfo(sb, qid->type)->dqi_priv, qid); + struct quota_info *dqopt = sb_dqopt(sb); + int ret; + + down_read(&dqopt->dqio_sem); + ret = qtree_get_next_id(sb_dqinfo(sb, qid->type)->dqi_priv, qid); + up_read(&dqopt->dqio_sem); + return ret; } static const struct quota_format_ops v2_format_ops = {
Push down acquisition of dqio_sem into ->get_next_id() callback. Mostly for consistency with other operations. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/quota/dquot.c | 6 +----- fs/quota/quota_v2.c | 8 +++++++- 2 files changed, 8 insertions(+), 6 deletions(-)