Re: [PATCH 1/8] quota: Allow to pass mount path to quotactl

From: Jan Kara <jack@suse.cz>
To: Christoph Hellwig <hch@infradead.org>
Cc: Jan Kara <jack@suse.cz>, Richard Weinberger <richard@nod.at>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	linux-mtd@lists.infradead.org, kernel@pengutronix.de,
	Jan Kara <jack@suse.com>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/8] quota: Allow to pass mount path to quotactl
Date: Tue, 26 Jan 2021 14:17:52 +0100	[thread overview]
Message-ID: <20210126131752.GB10966@quack2.suse.cz> (raw)
In-Reply-To: <20210125204249.GA1103662@infradead.org>

On Mon 25-01-21 20:42:49, Christoph Hellwig wrote:
> On Mon, Jan 25, 2021 at 04:45:07PM +0100, Jan Kara wrote:
> > > What do you mean by "take"? Take the superblock as an argument to
> > > quotactl_sb() or take a reference to the superblock?
> > > Sorry, I don't really get where you aiming at.
> > 
> > I think Christoph was pointing at the fact it is suboptimal to search for
> > superblock by device number when you already have a pointer to it.  And I
> > guess he was suggesting we could pass 'sb' pointer to quotactl_sb() when we
> > already have it. Although to be honest, I'm not sure how Christoph imagines
> > the refactoring of user_get_super() he mentions - when we have a path
> > looked up through user_path(), that pins the superblock the path is on so
> > it cannot be unmounted. So perhaps quotactl_sb() can done like:
> 
> I don't think we need a quotactl_sb at all, do_quotactl is in fact a
> pretty good abstraction as-is.
> 
> For the path based one we just need to factor out a little helper
> to set excl and thaw and then call it like:
> 
> 	sb = path.dentry->d_inode->i_sb;
> 
> 	if (excl)
> 		down_write(&sb->s_umount);
> 	else
> 		down_read(&sb->s_umount);
> 	if (thawed && sb->s_writers.frozen != SB_UNFROZEN)
> 		ret = -EBUSY;
> 	else
> 		ret = do_quotactl(sb, type, cmds, id, addr, &path);
> 	if (excl)
> 		up_write(&sb->s_umount);
> 	else
> 		up_read(&sb->s_umount);
> 
> as there is no good reason to bring over the somewhat strange wait until
> unfrozen semantics to a new syscall.

Well, I don't think that "wait until unfrozen" is that strange e.g. for
Q_SETQUOTA - it behaves like setxattr() or any other filesystem
modification operation. And IMO it is desirable that filesystem freezing is
transparent for operations like these. For stuff like Q_QUOTAON, I agree
that returning EBUSY makes sense but then I'm not convinced it's really
simpler or more useful behavior...

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/