| Message ID | 1442307754-13233-12-git-send-email-yangds.fnst@cn.fujitsu.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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. I don't like calling sync_filesystem() from quota code. Mainly, it does much more work than ->sync_fs() for most filesystems (it flushes all files to disk). Just adding a check for read-only filesystem would look better to me. Honza > --- > fs/quota/dquot.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c > index 140397a..0bda7fa 100644 > --- a/fs/quota/dquot.c > +++ b/fs/quota/dquot.c > @@ -668,8 +668,7 @@ int dquot_quota_sync(struct super_block *sb, int type) > /* This is not very clever (and fast) but currently I don't know about > * any other simple way of getting quota data to disk and we must get > * them there for userspace to be visible... */ > - if (sb->s_op->sync_fs) > - sb->s_op->sync_fs(sb, 1); > + sync_filesystem(sb); > sync_blockdev(sb->s_bdev); > > /* > @@ -2043,7 +2042,7 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) > /* > * Skip everything if there's nothing to do. We have to do this because > * sometimes we are called when fill_super() failed and calling > - * sync_fs() in such cases does no good. > + * sync_filesystem() in such cases does no good. > */ > if (!sb_any_quota_loaded(sb)) { > mutex_unlock(&dqopt->dqonoff_mutex); > @@ -2110,8 +2109,7 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) > > /* Sync the superblock so that buffers with quota data are written to > * disk (and so userspace sees correct data afterwards). */ > - if (sb->s_op->sync_fs) > - sb->s_op->sync_fs(sb, 1); > + sync_filesystem(sb); > sync_blockdev(sb->s_bdev); > /* Now the quota files are just ordinary files and we can set the > * inode flags back. Moreover we discard the pagecache so that > -- > 1.8.4.2 >
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? > > I don't like calling sync_filesystem() from quota code. Mainly, it does > much more work than ->sync_fs() for most filesystems (it flushes all files > to disk). Just adding a check for read-only filesystem would look better to > me. Okey, I will add a read-only check here. Thanx Yang > > Honza >> --- >> fs/quota/dquot.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c >> index 140397a..0bda7fa 100644 >> --- a/fs/quota/dquot.c >> +++ b/fs/quota/dquot.c >> @@ -668,8 +668,7 @@ int dquot_quota_sync(struct super_block *sb, int type) >> /* This is not very clever (and fast) but currently I don't know about >> * any other simple way of getting quota data to disk and we must get >> * them there for userspace to be visible... */ >> - if (sb->s_op->sync_fs) >> - sb->s_op->sync_fs(sb, 1); >> + sync_filesystem(sb); >> sync_blockdev(sb->s_bdev); >> >> /* >> @@ -2043,7 +2042,7 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) >> /* >> * Skip everything if there's nothing to do. We have to do this because >> * sometimes we are called when fill_super() failed and calling >> - * sync_fs() in such cases does no good. >> + * sync_filesystem() in such cases does no good. >> */ >> if (!sb_any_quota_loaded(sb)) { >> mutex_unlock(&dqopt->dqonoff_mutex); >> @@ -2110,8 +2109,7 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) >> >> /* Sync the superblock so that buffers with quota data are written to >> * disk (and so userspace sees correct data afterwards). */ >> - if (sb->s_op->sync_fs) >> - sb->s_op->sync_fs(sb, 1); >> + sync_filesystem(sb); >> sync_blockdev(sb->s_bdev); >> /* Now the quota files are just ordinary files and we can set the >> * inode flags back. Moreover we discard the pagecache so that >> -- >> 1.8.4.2 >>
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. 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... Honza
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c index 140397a..0bda7fa 100644 --- a/fs/quota/dquot.c +++ b/fs/quota/dquot.c @@ -668,8 +668,7 @@ int dquot_quota_sync(struct super_block *sb, int type) /* This is not very clever (and fast) but currently I don't know about * any other simple way of getting quota data to disk and we must get * them there for userspace to be visible... */ - if (sb->s_op->sync_fs) - sb->s_op->sync_fs(sb, 1); + sync_filesystem(sb); sync_blockdev(sb->s_bdev); /* @@ -2043,7 +2042,7 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) /* * Skip everything if there's nothing to do. We have to do this because * sometimes we are called when fill_super() failed and calling - * sync_fs() in such cases does no good. + * sync_filesystem() in such cases does no good. */ if (!sb_any_quota_loaded(sb)) { mutex_unlock(&dqopt->dqonoff_mutex); @@ -2110,8 +2109,7 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags) /* Sync the superblock so that buffers with quota data are written to * disk (and so userspace sees correct data afterwards). */ - if (sb->s_op->sync_fs) - sb->s_op->sync_fs(sb, 1); + sync_filesystem(sb); sync_blockdev(sb->s_bdev); /* Now the quota files are just ordinary files and we can set the * inode flags back. Moreover we discard the pagecache so that
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> --- fs/quota/dquot.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)