Re: [PATCH v3 11/39] fs: quota: replace opened calling of ->sync_fs with sync_filesystem

[Dongsheng Yang <yangds.fnst@cn.fujitsu.com>]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

On 09/17/2015 07:05 PM, Jan Kara wrote:
> On Thu 17-09-15 14:28:26, Dongsheng Yang wrote:
>> On 09/16/2015 06:14 PM, Jan Kara wrote:
>>> On Tue 15-09-15 17:02:06, Dongsheng Yang wrote:
>>>> There are only two places in whole kernel to call ->sync_fs directly. It
>>>> will sync fs even in read-only mode. It's not a good idea and some filesystem
>>>> would warn out if you are syncing in read-only mode. But sync_filesystem()
>>>> does filter this case out. Let's call sync_filesystem() here instead.
>>>>
>>>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>>>
>>> So I'd prefer ubifs used hidden system inodes for quota files, set
>>> DQUOT_QUOTA_SYS_FILE flag and so this code calling sync_fs() could be
>>> completely avoided.
>>
>> Hmmm, I think it's a good idea to make quota files SYS_FILEs. But how
>> about quota-tools? E.g, if quotacheck do some modification
>> on quota files, we would not read them without a sync_fs().
>>
>> Could you help explain how quota works in this case?
>
> So tools like quota(1) or setquota(1) work using quotactl so they don't
> need access to quota files. When quota files are system files, quotacheck
> functionality belongs into the fsck - so fsck.ubifs is responsible for
> checking consistency of quota files. How e.g. e2fsck does it is that when
> scanning inodes, it computes usage for each user / group, loads limits
> information from old quota files and then just creates new quota files with
> updated information if there's any difference to the old quota files.

About quotacheck, I think just call fsync() in it sounds good to me.
Maybe something like the attachment.

OKEY, but I found repquota is still using read() to access quota files.
Please consider that case: 1.we read quota file and there is a pagecache
for it. 2. Then kernel did some modification on quota files. But
if the files is SYS_FILES marked, dquot_quota_sync() would not drop
the pagecache, then 3. repquota would get an outdated data from pagecache.

I am not sure why ext4 works well in this case. There must be something
I am missing.

Maybe we can introduce a Q_GETBLK for quotactl() to make all
quota tools getting informations from ioctl.
>
> Another advantage of the checking functionality being in fsck is that
> fs-specific fsck can gather usage information much more efficiently (and
> fsck has to do full fs scan anyway) and there's no need to propagate quota
> usage information to userspace using FIQSIZE ioctl() and similar stuff...

So, let me try to summary the my opinions about it.
Pros:
	(1). Security. quota files shouldn't be accessible to usespace.
	(2). Efficiency. No need for quotacheck, just do it in fsck.
	(3). No need FIOQSIZE any more.
Cons:
	(1). Incompatibility:
		If I set DQUOT_QUOTA_SYS_FILE currently, there are at
least two commands would not work: quotacheck and repquota. Actually
that means the whole quota is not usable to user.

So, I think the compatibility is important to me. Then what about
setting quota files as regular files at first. After all tools (quota
tools, quotacheck, repquota, fsck) prepared, setting the
DQUOT_QUOTA_SYS_FILE seems better.

Yang
>
> 								Honza
>


[-- Attachment #2: 0001-quotacheck-sync-files-in-quotacheck.patch --]
[-- Type: text/x-patch, Size: 1100 bytes --]

>From 1cf9d072cdd4d17c620834864f79ec5a0765d2bf Mon Sep 17 00:00:00 2001
From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Date: Fri, 18 Sep 2015 08:40:49 -0400
Subject: [PATCH] quotacheck: sync files in quotacheck.

After a quotacheck, we need to make kernel to see the modification.
Compared with syncing quota files in kernel, we would rather sync
it in quotacheck command.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
---
 quotacheck.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/quotacheck.c b/quotacheck.c
index 6f34cff..e4b184d 100644
--- a/quotacheck.c
+++ b/quotacheck.c
@@ -768,6 +768,20 @@ rename_new:
 		close(fd);
 	}
 #endif
+
+	/* Do a syncing for the quota file */
+	if ((fd = open(filename, O_RDWR)) < 0) {
+		errstr(_("Cannot open new quota file %s: %s\n"), filename, strerror(errno));
+		free(filename);
+		return -1;
+	}
+        if (fsync(fd)) {
+		errstr(_("Cannot sync quota file %s: %s\n"), filename, strerror(errno));
+		free(filename);
+                return -1;
+        }
+	close(fd);
+
 	free(filename);
 	return 0;
 }
-- 
1.8.3.1