| Message ID | 1433758060-18614-1-git-send-email-yangds.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 08.06.2015 um 12:07 schrieb Dongsheng Yang: > Currently, ubifs does not support access time anyway. I understand > that there is a overhead to update inode in each access from user. > > But for the following two reasons, I think we can make it optional > to user. > > (1). More and more flash storage in server are trying to use ubifs, > it is not only for a device such as mobile phone any more, we want > to use it in more and more generic way. Then we need to compete > with some other main filesystems. From this point, access time is > necessary to us, at least as a choice to user currently. > > (2). The default mount option about atime is relatime currently, > it's much relaxy compared with strictatime. Then we don't update > the inode in any accessing. So the overhead is not too much. > It's really acceptable. > > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > --- > It's a RESEND patch to cc to fsdevel as Artem suggested. > I would rename force_atime to enable_atime in next version. > > fs/ubifs/file.c | 4 ++++ > fs/ubifs/super.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- > fs/ubifs/ubifs.h | 5 +++++ > 3 files changed, 58 insertions(+), 3 deletions(-) > > diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c > index 35efc10..e683890 100644 > --- a/fs/ubifs/file.c > +++ b/fs/ubifs/file.c > @@ -1541,11 +1541,15 @@ static const struct vm_operations_struct ubifs_file_vm_ops = { > static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma) > { > int err; > + struct inode *inode = file->f_mapping->host; > + struct ubifs_info *c = inode->i_sb->s_fs_info; > > err = generic_file_mmap(file, vma); > if (err) > return err; > vma->vm_ops = &ubifs_file_vm_ops; > + if (c->force_atime) > + file_accessed(file); > return 0; > } > > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c > index 75e6f04..8c2773b 100644 > --- a/fs/ubifs/super.c > +++ b/fs/ubifs/super.c > @@ -128,7 +128,9 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum) > if (err) > goto out_ino; > > - inode->i_flags |= (S_NOCMTIME | S_NOATIME); > + inode->i_flags |= S_NOCMTIME; > + if (!c->force_atime) > + inode->i_flags |= S_NOATIME; > set_nlink(inode, le32_to_cpu(ino->nlink)); > i_uid_write(inode, le32_to_cpu(ino->uid)); > i_gid_write(inode, le32_to_cpu(ino->gid)); > @@ -378,15 +380,47 @@ done: > clear_inode(inode); > } > > +/* > + * There is only one possible caller of ubifs_dirty_inode without holding > + * ui->ui_mutex, file_accessed. We are going to support atime if user > + * set the mount option of force_atime. In that case, ubifs_dirty_inode > + * need to lock ui->ui_mutex by itself and do a budget by itself. > + */ > static void ubifs_dirty_inode(struct inode *inode, int flags) > { > struct ubifs_inode *ui = ubifs_inode(inode); > + int locked = mutex_is_locked(&ui->ui_mutex); > + struct ubifs_info *c = inode->i_sb->s_fs_info; > + int ret = 0; > + > + if (!locked) > + mutex_lock(&ui->ui_mutex); Please try as hard as possible to avoid conditional locking. Thanks, //richard -- 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
Am 08.06.2015 um 12:07 schrieb Dongsheng Yang: > - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); > if (!ui->dirty) { > + if (!locked) { > + /* > + * It's a little tricky here, there is only one > + * possible user of ubifs_dirty_inode did not do > + * a budget for this inode. At the same time, this > + * user is not holding the ui->ui_mutex. Then if > + * we found ui->ui_mutex is not locked, we can say: > + * we need to do a budget in ubifs_dirty_inode here. > + */ > + struct ubifs_budget_req req = { .dirtied_ino = 1, > + .dirtied_ino_d = ALIGN(ui->data_len, 8) }; > + > + ret = ubifs_budget_space(c, &req); > + if (ret) > + goto out; > + } So, this is the new case when ->dirty_inode() is called via generic_update_time()? Did you research whether you can detect that case also by looking at the flags parameter? I'd give I_DIRTY_TIME a try. This way you could get at least rid of the mutex_is_locked() usage. Thanks, //richard -- 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
On 06/09/2015 06:55 AM, Richard Weinberger wrote: > Am 08.06.2015 um 12:07 schrieb Dongsheng Yang: >> - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); >> if (!ui->dirty) { >> + if (!locked) { >> + /* >> + * It's a little tricky here, there is only one >> + * possible user of ubifs_dirty_inode did not do >> + * a budget for this inode. At the same time, this >> + * user is not holding the ui->ui_mutex. Then if >> + * we found ui->ui_mutex is not locked, we can say: >> + * we need to do a budget in ubifs_dirty_inode here. >> + */ >> + struct ubifs_budget_req req = { .dirtied_ino = 1, >> + .dirtied_ino_d = ALIGN(ui->data_len, 8) }; >> + >> + ret = ubifs_budget_space(c, &req); >> + if (ret) >> + goto out; >> + } > > So, this is the new case when ->dirty_inode() is called via generic_update_time()? > Did you research whether you can detect that case also by looking at the flags parameter? > I'd give I_DIRTY_TIME a try. This way you could get at least rid of the mutex_is_locked() > usage. Thanx Richard, yes, we can use it to check if we need a budget here. But without removing the conditional lock as you suggested in last mail, that would not help a lot. Then I think I need to try more to find a better way about this feature by looking more into the flags. :) Thanx Yang > > Thanks, > //richard > . > -- 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
On 06/09/2015 06:55 AM, Richard Weinberger wrote: > Am 08.06.2015 um 12:07 schrieb Dongsheng Yang: >> - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); >> if (!ui->dirty) { >> + if (!locked) { >> + /* >> + * It's a little tricky here, there is only one >> + * possible user of ubifs_dirty_inode did not do >> + * a budget for this inode. At the same time, this >> + * user is not holding the ui->ui_mutex. Then if >> + * we found ui->ui_mutex is not locked, we can say: >> + * we need to do a budget in ubifs_dirty_inode here. >> + */ >> + struct ubifs_budget_req req = { .dirtied_ino = 1, >> + .dirtied_ino_d = ALIGN(ui->data_len, 8) }; >> + >> + ret = ubifs_budget_space(c, &req); >> + if (ret) >> + goto out; >> + } > > So, this is the new case when ->dirty_inode() is called via generic_update_time()? > Did you research whether you can detect that case also by looking at the flags parameter? > I'd give I_DIRTY_TIME a try. This way you could get at least rid of the mutex_is_locked() > usage. Okey, after a reading, I'm afraid I can not think a better idea out. The flags between *old* cases and the *new* case can possiblly be same. Then we can't use the flags to filter the new case from old cases. But I think I can append a patch to add a support for lazytime here: if (flags == I_DIRTY_TIME) return; Thanx Yang > > Thanks, > //richard > . > -- 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
On 06/09/2015 11:24 AM, Dongsheng Yang wrote: > On 06/09/2015 06:55 AM, Richard Weinberger wrote: >> Am 08.06.2015 um 12:07 schrieb Dongsheng Yang: >>> - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); >>> if (!ui->dirty) { >>> + if (!locked) { >>> + /* >>> + * It's a little tricky here, there is only one >>> + * possible user of ubifs_dirty_inode did not do >>> + * a budget for this inode. At the same time, this >>> + * user is not holding the ui->ui_mutex. Then if >>> + * we found ui->ui_mutex is not locked, we can say: >>> + * we need to do a budget in ubifs_dirty_inode here. >>> + */ >>> + struct ubifs_budget_req req = { .dirtied_ino = 1, >>> + .dirtied_ino_d = ALIGN(ui->data_len, 8) }; >>> + >>> + ret = ubifs_budget_space(c, &req); >>> + if (ret) >>> + goto out; >>> + } >> >> So, this is the new case when ->dirty_inode() is called via >> generic_update_time()? >> Did you research whether you can detect that case also by looking at >> the flags parameter? >> I'd give I_DIRTY_TIME a try. This way you could get at least rid of >> the mutex_is_locked() >> usage. > > Okey, after a reading, I'm afraid I can not think a better idea > out. The flags between *old* cases and the *new* case can possiblly > be same. Then we can't use the flags to filter the new case from old > cases. Oops, sorry, my bad!! generic_upadte_time() is the only way to use I_DIRTY_TIME here currently. You are right!. we can get rid of mutex_is_locked() at least. Thanx Yang > > But I think I can append a patch to add a support for lazytime here: > if (flags == I_DIRTY_TIME) > return; > > Thanx > Yang > >> >> Thanks, >> //richard >> . >> > > -- > 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 > . > -- 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
On 06/09/2015 01:00 PM, Dongsheng Yang wrote: > On 06/09/2015 11:24 AM, Dongsheng Yang wrote: >> On 06/09/2015 06:55 AM, Richard Weinberger wrote: >>> Am 08.06.2015 um 12:07 schrieb Dongsheng Yang: >>>> - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); >>>> if (!ui->dirty) { >>>> + if (!locked) { >>>> + /* >>>> + * It's a little tricky here, there is only one >>>> + * possible user of ubifs_dirty_inode did not do >>>> + * a budget for this inode. At the same time, this >>>> + * user is not holding the ui->ui_mutex. Then if >>>> + * we found ui->ui_mutex is not locked, we can say: >>>> + * we need to do a budget in ubifs_dirty_inode here. >>>> + */ >>>> + struct ubifs_budget_req req = { .dirtied_ino = 1, >>>> + .dirtied_ino_d = ALIGN(ui->data_len, 8) }; >>>> + >>>> + ret = ubifs_budget_space(c, &req); >>>> + if (ret) >>>> + goto out; >>>> + } >>> >>> So, this is the new case when ->dirty_inode() is called via >>> generic_update_time()? >>> Did you research whether you can detect that case also by looking at >>> the flags parameter? >>> I'd give I_DIRTY_TIME a try. This way you could get at least rid of >>> the mutex_is_locked() >>> usage. >> >> Okey, after a reading, I'm afraid I can not think a better idea >> out. The flags between *old* cases and the *new* case can possiblly >> be same. Then we can't use the flags to filter the new case from old >> cases. > > Oops, sorry, my bad!! > > generic_upadte_time() is the only way to use I_DIRTY_TIME here currently. But there is another problem, if we are going to try to support lazytime, we have to set the I_DIRTY_TIME in the *old* cases. Then we can not use the flags to distinguish them in drity_inode(). > You are right!. we can get rid of mutex_is_locked() at least. > > Thanx > Yang >> >> But I think I can append a patch to add a support for lazytime here: >> if (flags == I_DIRTY_TIME) >> return; >> >> Thanx >> Yang >> >>> >>> Thanks, >>> //richard >>> . >>> >> >> -- >> 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 >> . >> > > -- > 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 > . > -- 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
On Mon, 2015-06-08 at 18:07 +0800, Dongsheng Yang wrote: > Currently, ubifs does not support access time anyway. I understand > that there is a overhead to update inode in each access from user. > > But for the following two reasons, I think we can make it optional > to user. > > (1). More and more flash storage in server are trying to use ubifs, > it is not only for a device such as mobile phone any more, we want > to use it in more and more generic way. Then we need to compete > with some other main filesystems. From this point, access time is > necessary to us, at least as a choice to user currently. > > (2). The default mount option about atime is relatime currently, > it's much relaxy compared with strictatime. Then we don't update > the inode in any accessing. So the overhead is not too much. > It's really acceptable. > > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > --- > It's a RESEND patch to cc to fsdevel as Artem suggested. > I would rename force_atime to enable_atime in next version. Why do you need to introduce a custom "force_atime" option if there are already standard "atime" and "noatime" mount option? I am fine with adding atime support to UBIFS in general, and I'd expect this behavior then. 1. mount -t ubifs ... - no atime by default 2. mount -t ubifs -o noatime ... - same as above 3. mount -t ubifs -o atime - support atime 4. mount -t ubifs -o rlatime - support relatime and so on for as many atime update strategies you want to support. -- 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
Am 09.06.2015 um 08:36 schrieb Artem Bityutskiy: > On Mon, 2015-06-08 at 18:07 +0800, Dongsheng Yang wrote: >> Currently, ubifs does not support access time anyway. I understand >> that there is a overhead to update inode in each access from user. >> >> But for the following two reasons, I think we can make it optional >> to user. >> >> (1). More and more flash storage in server are trying to use ubifs, >> it is not only for a device such as mobile phone any more, we want >> to use it in more and more generic way. Then we need to compete >> with some other main filesystems. From this point, access time is >> necessary to us, at least as a choice to user currently. >> >> (2). The default mount option about atime is relatime currently, >> it's much relaxy compared with strictatime. Then we don't update >> the inode in any accessing. So the overhead is not too much. >> It's really acceptable. >> >> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >> --- >> It's a RESEND patch to cc to fsdevel as Artem suggested. >> I would rename force_atime to enable_atime in next version. > > Why do you need to introduce a custom "force_atime" option if there are > already standard "atime" and "noatime" mount option? I am fine with > adding atime support to UBIFS in general, and I'd expect this behavior > then. I think the rationale behind force_atime was "I know atime can hurt my NAND and I know what I'm doing". :-) Such that possible users think of the consequences. Thanks, //richard -- 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
On 06/09/2015 04:02 PM, Richard Weinberger wrote: > Am 09.06.2015 um 08:36 schrieb Artem Bityutskiy: >> On Mon, 2015-06-08 at 18:07 +0800, Dongsheng Yang wrote: >>> Currently, ubifs does not support access time anyway. I understand >>> that there is a overhead to update inode in each access from user. >>> >>> But for the following two reasons, I think we can make it optional >>> to user. >>> >>> (1). More and more flash storage in server are trying to use ubifs, >>> it is not only for a device such as mobile phone any more, we want >>> to use it in more and more generic way. Then we need to compete >>> with some other main filesystems. From this point, access time is >>> necessary to us, at least as a choice to user currently. >>> >>> (2). The default mount option about atime is relatime currently, >>> it's much relaxy compared with strictatime. Then we don't update >>> the inode in any accessing. So the overhead is not too much. >>> It's really acceptable. >>> >>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >>> --- >>> It's a RESEND patch to cc to fsdevel as Artem suggested. >>> I would rename force_atime to enable_atime in next version. >> >> Why do you need to introduce a custom "force_atime" option if there are >> already standard "atime" and "noatime" mount option? I am fine with >> adding atime support to UBIFS in general, and I'd expect this behavior >> then. > > I think the rationale behind force_atime was "I know atime can hurt my NAND and I know what > I'm doing". :-) > Such that possible users think of the consequences. Thanx Richard, Yes, that's my point. :-) In addition, the atime and noatime are non-fs dependent options. Then these options would be parsed in userspace and vfs will get a flags about them. vfs are treating default as atime enabled, then vfs will set MNT_RELATIME in flags: 2592 /* Default to relatime unless overriden */ 2593 if (!(flags & MS_NOATIME)) 2594 mnt_flags |= MNT_RELATIME; But ubifs is working differently. ubifs disables atime by default. The problem is, we can not distinguish the following two case in ubifs. 1. mount -t ubifs ... - MNT_RELATIME in flags 2. mount -t ubifs -o atime - MNT_RELATIME in flags too In vfs, they are equal. In ubifs, we want different behaviours but we can not distinguish them. Therefore, I introduced a new option named as force_atime in ubifs. That's a ubifs-dependent opiton and it works as a main switch, in a higher level compared with atime and noatime. If force_atime, we support the atime-related flags. Otherwise, we don't care about all of them in flags and don't support atime anyway. Thanx Yang > > Thanks, > //richard > . > -- 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
On Wed, 2015-06-10 at 11:16 +0800, Dongsheng Yang wrote: > Therefore, I introduced a new option named as force_atime in ubifs. > That's a ubifs-dependent opiton and it works as a main switch, in > a higher level compared with atime and noatime. If force_atime, we > support the atime-related flags. Otherwise, we don't care about all of > them in flags and don't support atime anyway. How bad is it to just default to relatime like other file-systems do, comparing to what we have now? -- 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
On 06/10/2015 05:21 PM, Artem Bityutskiy wrote: > On Wed, 2015-06-10 at 11:16 +0800, Dongsheng Yang wrote: >> Therefore, I introduced a new option named as force_atime in ubifs. >> That's a ubifs-dependent opiton and it works as a main switch, in >> a higher level compared with atime and noatime. If force_atime, we >> support the atime-related flags. Otherwise, we don't care about all of >> them in flags and don't support atime anyway. > > How bad is it to just default to relatime like other file-systems do, > comparing to what we have now? Ha, yes, that's a problem. I read it from wiki that the author think it's bad for ubifs. But I did not do a measure about it. In theory, yes, lots of writing would damage the flash. So I think just make it optional to user is a flexible way to do it. Even we want to make the default to relatime, I think it's better to keep the compatibility for a period and provide a force_atime to user. When lots of users said "okey, we are mostly choosing force_atime in our use cases.". I believe that's a safe way to make ubifs supporting atime by default. > > . > -- 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
On Wed, 2015-06-10 at 18:10 +0800, Dongsheng Yang wrote: > On 06/10/2015 05:21 PM, Artem Bityutskiy wrote: > > On Wed, 2015-06-10 at 11:16 +0800, Dongsheng Yang wrote: > >> Therefore, I introduced a new option named as force_atime in ubifs. > >> That's a ubifs-dependent opiton and it works as a main switch, in > >> a higher level compared with atime and noatime. If force_atime, we > >> support the atime-related flags. Otherwise, we don't care about all of > >> them in flags and don't support atime anyway. > > > > How bad is it to just default to relatime like other file-systems do, > > comparing to what we have now? > > Ha, yes, that's a problem. I read it from wiki that the author think > it's bad for ubifs. But I did not do a measure about it. Since I am one of the authors, I think we were mostly looking at the full atime support, and did not really look at relatime. > In theory, yes, lots of writing would damage the flash. So I think > just make it optional to user is a flexible way to do it. Even we > want to make the default to relatime, I think it's better to keep > the compatibility for a period and provide a force_atime to user. > > When lots of users said "okey, we are mostly choosing force_atime in our > use cases.". I believe that's a safe way to make ubifs supporting > atime by default. Let me make a step back. So what I hear is that the problem is that you cannot find the original mount options. For example, when you see the MNT_RELATIME flag, you do not know whether it was specified by the user or it was VFS adding this flag. Is this correct? If it is correct, then I think we need to look at a VFS-level solution. If the above is the only problem, then I'd say that introducing a custom "force_atime" is a work-around for VFS limitations. Artem. -- 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
On 06/10/2015 06:25 PM, Artem Bityutskiy wrote: > On Wed, 2015-06-10 at 18:10 +0800, Dongsheng Yang wrote: >> On 06/10/2015 05:21 PM, Artem Bityutskiy wrote: >>> On Wed, 2015-06-10 at 11:16 +0800, Dongsheng Yang wrote: >>>> Therefore, I introduced a new option named as force_atime in ubifs. >>>> That's a ubifs-dependent opiton and it works as a main switch, in >>>> a higher level compared with atime and noatime. If force_atime, we >>>> support the atime-related flags. Otherwise, we don't care about all of >>>> them in flags and don't support atime anyway. >>> >>> How bad is it to just default to relatime like other file-systems do, >>> comparing to what we have now? >> >> Ha, yes, that's a problem. I read it from wiki that the author think >> it's bad for ubifs. But I did not do a measure about it. > > Since I am one of the authors, I think we were mostly looking at the > full atime support, and did not really look at relatime. > >> In theory, yes, lots of writing would damage the flash. So I think >> just make it optional to user is a flexible way to do it. Even we >> want to make the default to relatime, I think it's better to keep >> the compatibility for a period and provide a force_atime to user. >> >> When lots of users said "okey, we are mostly choosing force_atime in our >> use cases.". I believe that's a safe way to make ubifs supporting >> atime by default. > > Let me make a step back. So what I hear is that the problem is that you > cannot find the original mount options. For example, when you see the > MNT_RELATIME flag, you do not know whether it was specified by the user > or it was VFS adding this flag. Is this correct? > > If it is correct, then I think we need to look at a VFS-level solution. > If the above is the only problem, then I'd say that introducing a custom > "force_atime" is a work-around for VFS limitations. That's correct. Yes, I really want to solve it in vfs at first. But later, just submited this patch as a Problem-solved for us. Because I thought the force_atime would disappear when we decide to support atime by default in future. Besides a change in VFS would cause more discussion, after a trade-off, I submitted this patch for ubifs. :) But yes, there is really, at leat, a TODO entry for VFS in this scenario I think. If you think we need to do it rather than a work-around as what this patch did. I will think a better way in VFS for that. :) Thanx Yang > > Artem. > > . > -- 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
On Wed, 2015-06-10 at 18:34 +0800, Dongsheng Yang wrote: > On 06/10/2015 06:25 PM, Artem Bityutskiy wrote: > > On Wed, 2015-06-10 at 18:10 +0800, Dongsheng Yang wrote: > >> On 06/10/2015 05:21 PM, Artem Bityutskiy wrote: > >>> On Wed, 2015-06-10 at 11:16 +0800, Dongsheng Yang wrote: > >>>> Therefore, I introduced a new option named as force_atime in ubifs. > >>>> That's a ubifs-dependent opiton and it works as a main switch, in > >>>> a higher level compared with atime and noatime. If force_atime, we > >>>> support the atime-related flags. Otherwise, we don't care about all of > >>>> them in flags and don't support atime anyway. > >>> > >>> How bad is it to just default to relatime like other file-systems do, > >>> comparing to what we have now? > >> > >> Ha, yes, that's a problem. I read it from wiki that the author think > >> it's bad for ubifs. But I did not do a measure about it. > > > > Since I am one of the authors, I think we were mostly looking at the > > full atime support, and did not really look at relatime. > > > >> In theory, yes, lots of writing would damage the flash. So I think > >> just make it optional to user is a flexible way to do it. Even we > >> want to make the default to relatime, I think it's better to keep > >> the compatibility for a period and provide a force_atime to user. > >> > >> When lots of users said "okey, we are mostly choosing force_atime in our > >> use cases.". I believe that's a safe way to make ubifs supporting > >> atime by default. > > > > Let me make a step back. So what I hear is that the problem is that you > > cannot find the original mount options. For example, when you see the > > MNT_RELATIME flag, you do not know whether it was specified by the user > > or it was VFS adding this flag. Is this correct? > > > > If it is correct, then I think we need to look at a VFS-level solution. > > If the above is the only problem, then I'd say that introducing a custom > > "force_atime" is a work-around for VFS limitations. > > That's correct. Yes, I really want to solve it in vfs at first. But > later, just submited this patch as a Problem-solved for us. Because I > thought the force_atime would disappear when we decide to support > atime by default in future. > > Besides a change in VFS would cause more discussion, after a trade-off, > I submitted this patch for ubifs. :) > > But yes, there is really, at leat, a TODO entry for VFS in this > scenario I think. If you think we need to do it rather than a > work-around as what this patch did. I will think a better way > in VFS for that. :) Yes, I think a custom mount option should be the last resort solution, for the case when other options failed. One way would be to push this assignment down to individual file-systems. Another way would be to have the original flags preserved and passed to the file-system. May be you can find a better way. -- 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
On 06/10/2015 07:05 PM, Artem Bityutskiy wrote: > On Wed, 2015-06-10 at 18:34 +0800, Dongsheng Yang wrote: >> On 06/10/2015 06:25 PM, Artem Bityutskiy wrote: >>> On Wed, 2015-06-10 at 18:10 +0800, Dongsheng Yang wrote: >>>> On 06/10/2015 05:21 PM, Artem Bityutskiy wrote: >>>>> On Wed, 2015-06-10 at 11:16 +0800, Dongsheng Yang wrote: >>>>>> Therefore, I introduced a new option named as force_atime in ubifs. >>>>>> That's a ubifs-dependent opiton and it works as a main switch, in >>>>>> a higher level compared with atime and noatime. If force_atime, we >>>>>> support the atime-related flags. Otherwise, we don't care about all of >>>>>> them in flags and don't support atime anyway. >>>>> >>>>> How bad is it to just default to relatime like other file-systems do, >>>>> comparing to what we have now? >>>> >>>> Ha, yes, that's a problem. I read it from wiki that the author think >>>> it's bad for ubifs. But I did not do a measure about it. >>> >>> Since I am one of the authors, I think we were mostly looking at the >>> full atime support, and did not really look at relatime. >>> >>>> In theory, yes, lots of writing would damage the flash. So I think >>>> just make it optional to user is a flexible way to do it. Even we >>>> want to make the default to relatime, I think it's better to keep >>>> the compatibility for a period and provide a force_atime to user. >>>> >>>> When lots of users said "okey, we are mostly choosing force_atime in our >>>> use cases.". I believe that's a safe way to make ubifs supporting >>>> atime by default. >>> >>> Let me make a step back. So what I hear is that the problem is that you >>> cannot find the original mount options. For example, when you see the >>> MNT_RELATIME flag, you do not know whether it was specified by the user >>> or it was VFS adding this flag. Is this correct? >>> >>> If it is correct, then I think we need to look at a VFS-level solution. >>> If the above is the only problem, then I'd say that introducing a custom >>> "force_atime" is a work-around for VFS limitations. >> >> That's correct. Yes, I really want to solve it in vfs at first. But >> later, just submited this patch as a Problem-solved for us. Because I >> thought the force_atime would disappear when we decide to support >> atime by default in future. >> >> Besides a change in VFS would cause more discussion, after a trade-off, >> I submitted this patch for ubifs. :) >> >> But yes, there is really, at leat, a TODO entry for VFS in this >> scenario I think. If you think we need to do it rather than a >> work-around as what this patch did. I will think a better way >> in VFS for that. :) > > Yes, I think a custom mount option should be the last resort solution, > for the case when other options failed. > > One way would be to push this assignment down to individual > file-systems. Another way would be to have the original flags preserved > and passed to the file-system. > > May be you can find a better way. Hi Artem, sorry for the late. After the last discussion, I have been focus on some other work. But when I came back to this topic today, I found I was wrong. Even in vfs, unfortunately, we can not distinguish the use case 1 and 2: 1. mount -t ubifs ... - (flags & MS_NOATIME) == 0 2. mount -t ubifs -o atime - (flags & MS_NOATIME) == 0 3. mount -t ubifs -o noatime - (flags & MS_NOATIME) == 1 There is no MS_ATIME defined in vfs to be used for -o atime specified. We can only find out the case 3 by the flags in kernel space. That means, the information what we want for this topic only exists in util-linux in user-space. util-linux make the Default equal to -o atime. Then there are two ways to do it: a), introduce a MS_ATIME and file_system_type::parse_options to vfs. It's a little costly. MS_ATIME would be used to make kernel to be aware of the atime option. parse_options() could be used to make file system to be in charge of parsing the standard options and set the default value. parse_options() is a common requirement, but the MS_ATIME looks much arguable. b), introduce a force_atime to ubifs as what my patch is doing here. 1), If we really need to know atime option in vfs, I propose to experiment it in ubifs at first. If the force_atime works well and looks useful to some other file system. Then we can consider to introduce MS_ATIME to vfs. So, force_atime could be treat as an experiment for MS_ATIME. 2), As we talked before, I think force_atime is a flexible way to change the default value to relatime or future lazytime. Then I believe it's better to make the radialization limited in ubifs. c), change the default behaviour of ubifs to support atime immediately. That's rough and risky but most easy way to do it. In short, I think force_atime to ubifs is the choice from my opinion. What do you think about it? Thanx Yang > > . > -- 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
On Tue, 2015-06-23 at 17:55 +0800, Dongsheng Yang wrote:
> In short, I think force_atime to ubifs is the choice from my opinion.
So will we end up with this:
-o - no atime support
-o atime - no atime support
-o noatime - same, no atime support
-o force_atime - full atime support
-o relatime - relative atime support
-o lazyatime - lazy atime support
IOW, atime/noatime mount options have no effect on UBIFS. To have full
atime support - people have to use "force_atime". And then the rest of
the standard options are supported.
So if you are the user, would not you find this confusing and
inconsistent? I would.
How about this alternative: we preserve current behavior, but we
introduce a compile-time configuration option which enables atime
support _and_ changes the default behavior to match the behavior of the
mainstream file-systems.
Or to put it differently.
1. We introduce the UBIFS_ATIME_SUPPORT configuration option. This
option will be off by default.
3. If UBIFS_ATIME_SUPPORT is off, users get the current (legacy)
behavior. Atime is not supported. The atime/noatime/relatime/lazyatime
mount options are ignored.
4. If UBIFS_ATIME_SUPPORT is on, UBIFS supports atime by default. I.e.:
-o - full atime support
-o atime - full atime support
-o noatime - no atime support
We may also print a fat big warning from the mount function about the
fact that atime support is enabled. Just in case a legacy user enabled
this option.
How does this sound to you?
Artem.
--
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
On 06/23/2015 06:44 PM, Artem Bityutskiy wrote: > On Tue, 2015-06-23 at 17:55 +0800, Dongsheng Yang wrote: >> In short, I think force_atime to ubifs is the choice from my opinion. > > So will we end up with this: > > -o - no atime support > -o atime - no atime support > -o noatime - same, no atime support > -o force_atime - full atime support > -o relatime - relative atime support > -o lazyatime - lazy atime support > > IOW, atime/noatime mount options have no effect on UBIFS. To have full > atime support - people have to use "force_atime". And then the rest of > the standard options are supported. > > So if you are the user, would not you find this confusing and > inconsistent? I would. > > > How about this alternative: we preserve current behavior, but we > introduce a compile-time configuration option which enables atime > support _and_ changes the default behavior to match the behavior of the > mainstream file-systems. > > Or to put it differently. > > 1. We introduce the UBIFS_ATIME_SUPPORT configuration option. This > option will be off by default. > > 3. If UBIFS_ATIME_SUPPORT is off, users get the current (legacy) > behavior. Atime is not supported. The atime/noatime/relatime/lazyatime > mount options are ignored. > > 4. If UBIFS_ATIME_SUPPORT is on, UBIFS supports atime by default. I.e.: > > -o - full atime support > -o atime - full atime support > -o noatime - no atime support > > We may also print a fat big warning from the mount function about the > fact that atime support is enabled. Just in case a legacy user enabled > this option. > > How does this sound to you? Great!! good idea to me. And we can also do the other changes to match mainstream file-systems, if necessary, in similar way in future. Yang > > Artem. > > . > -- 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
On Tue, Jun 23, 2015 at 01:44:00PM +0300, Artem Bityutskiy wrote: > On Tue, 2015-06-23 at 17:55 +0800, Dongsheng Yang wrote: > > In short, I think force_atime to ubifs is the choice from my opinion. > > So will we end up with this: > > -o - no atime support > -o atime - no atime support > -o noatime - same, no atime support > -o force_atime - full atime support > -o relatime - relative atime support > -o lazyatime - lazy atime support > IOW, atime/noatime mount options have no effect on UBIFS. To have full > atime support - people have to use "force_atime". And then the rest of > the standard options are supported. That's the exact semantics of the standard -o strictatime option. See the mount(8) man page: strictatime Allows to explicitly requesting full atime updates. This makes it possible for kernel to defaults to relatime or noatime but still allow userspace to override it. For more details about the default system mount options see /proc/mounts. It's passed down to the kernel via the MS_STRICTATIME flag. All you need to do is make ubifs aware of this flag... Cheers, Dave.
On Wed, 2015-06-24 at 10:33 +1000, Dave Chinner wrote: > It's passed down to the kernel via the MS_STRICTATIME flag. All > you need to do is make ubifs aware of this flag... Oh, right, thank you. Indeed this is it. -- 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
On 06/24/2015 08:33 AM, Dave Chinner wrote: > On Tue, Jun 23, 2015 at 01:44:00PM +0300, Artem Bityutskiy wrote: >> On Tue, 2015-06-23 at 17:55 +0800, Dongsheng Yang wrote: >>> In short, I think force_atime to ubifs is the choice from my opinion. >> >> So will we end up with this: >> >> -o - no atime support >> -o atime - no atime support >> -o noatime - same, no atime support >> -o force_atime - full atime support >> -o relatime - relative atime support >> -o lazyatime - lazy atime support > >> IOW, atime/noatime mount options have no effect on UBIFS. To have full >> atime support - people have to use "force_atime". And then the rest of >> the standard options are supported. > > That's the exact semantics of the standard -o strictatime option. > See the mount(8) man page: > > strictatime > Allows to explicitly requesting full atime updates. > This makes it possible for kernel to defaults to > relatime or noatime but still allow userspace to > override it. For more details about the default system > mount options see /proc/mounts. > > It's passed down to the kernel via the MS_STRICTATIME flag. All > you need to do is make ubifs aware of this flag... Hi Dave, thanx for your suggestiong, but sorry, it's a little confusing to me :(. From the sentence of manpage, I assume this history: (1). Long time ago, atime is updated at *any* access. It was working as strictatime mode, although at that time there was a strictatime option for mount. (2). Later, we introduced a relatime for more relaxy atime updating. But the default mode was still full atime updating. (3). Later than later, 2.6.30, we want to make the default mode to relatime. But we want to provide a option to user to use the full atime updating. Then we introduced the strictatime option to mount. So the manpage of: "This makes it possible for kernel to defaults to relatime or noatime but still allow userspace to override it." means, we are going to change the default behaviour to relatime but we provide a option named as strictatime to user to explicitly request full atime updating. So, strictatime is in the same level with relatime, lazytime. They all are strategies for atime updating. But that's not what we want here. If I understand your suggestion correctly here, you are going to make ubifs to reuse the option of strictatime to work as what I proposed force_atime. Oops, I think found the problem, maybe it's about the expression from Atem: -o - no atime support -o atime - no atime support -o noatime - same, no atime support -o force_atime - full atime support <------- criminal -o relatime - relative atime support -o lazyatime - lazy atime support I guess you think the "full atime support" means "full atime updating strategy" what strictatime option stands for. But please read the later words from Atem: "people have to use "force_atime". And then the rest of the standard options are supported." Let me show this list again: -o - no atime support -o atime - no atime support -o noatime - same, no atime support -o force_atime - default behavior (relatime currently) -o force_atime,relatime - relative atime support -o force_atime,strictatime - strict atime support -o force_atime,lazyatime - lazy atime support Actually, force_atime is not equal with strictatime for full atime updating. It's a switch in higher level. If we enable force_atime, ubifs will support the all strategies user requested, default to relatime currently. But now, I agree with the idea from Atem to make it configurable in compiling time, introducing UBIFS_ATIME_SUPPORT. I wish I found the reason of misunderstanding and expressed myself clearly. Thanx Yang > > Cheers, > > Dave. > -- 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
On Thu, 2015-06-25 at 17:55 +0800, Dongsheng Yang wrote: > On 06/24/2015 08:33 AM, Dave Chinner wrote: > > On Tue, Jun 23, 2015 at 01:44:00PM +0300, Artem Bityutskiy wrote: > >> On Tue, 2015-06-23 at 17:55 +0800, Dongsheng Yang wrote: > >>> In short, I think force_atime to ubifs is the choice from my opinion. > >> > >> So will we end up with this: > >> > >> -o - no atime support > >> -o atime - no atime support > >> -o noatime - same, no atime support > >> -o force_atime - full atime support > >> -o relatime - relative atime support > >> -o lazyatime - lazy atime support > > > >> IOW, atime/noatime mount options have no effect on UBIFS. To have full > >> atime support - people have to use "force_atime". And then the rest of > >> the standard options are supported. > > > > That's the exact semantics of the standard -o strictatime option. > > See the mount(8) man page: > > > > strictatime > > Allows to explicitly requesting full atime updates. > > This makes it possible for kernel to defaults to > > relatime or noatime but still allow userspace to > > override it. For more details about the default system > > mount options see /proc/mounts. > > > > It's passed down to the kernel via the MS_STRICTATIME flag. All > > you need to do is make ubifs aware of this flag... > > Hi Dave, thanx for your suggestiong, but sorry, it's a little confusing > to me :(. I do not know the history, but IIUC, this is what Dave's hint translates to for UBIFS: -o - default behavior (no atime) -o atime - default behavior (no atime) -o noatime - default behavior (no atime) -o strictatime - full atime support -o relatime - relative atime support -o lazyatime - lazy atime support Is this logical from user's perspective? No, but this is a standard "hack", not an UBIFS-only "hack", so we are fine. "force_atime" that you are suggesting would be UBIFS-only hack, which is not as fine as a standard and documented "hack". IOW, atime/noatime are the "don't use" options, they are ignored and every file-system is free to use its own defaults, be that noatime or relatime or strictatime. If you want to tell the FS what to do, use strictatime/relatime/lazyatime. Does it make sense? -- 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
On 06/25/2015 06:08 PM, Artem Bityutskiy wrote: > > > On Thu, 2015-06-25 at 17:55 +0800, Dongsheng Yang wrote: >> On 06/24/2015 08:33 AM, Dave Chinner wrote: >>> On Tue, Jun 23, 2015 at 01:44:00PM +0300, Artem Bityutskiy wrote: >>>> On Tue, 2015-06-23 at 17:55 +0800, Dongsheng Yang wrote: >>>>> In short, I think force_atime to ubifs is the choice from my opinion. >>>> >>>> So will we end up with this: >>>> >>>> -o - no atime support >>>> -o atime - no atime support >>>> -o noatime - same, no atime support >>>> -o force_atime - full atime support >>>> -o relatime - relative atime support >>>> -o lazyatime - lazy atime support >>> >>>> IOW, atime/noatime mount options have no effect on UBIFS. To have full >>>> atime support - people have to use "force_atime". And then the rest of >>>> the standard options are supported. >>> >>> That's the exact semantics of the standard -o strictatime option. >>> See the mount(8) man page: >>> >>> strictatime >>> Allows to explicitly requesting full atime updates. >>> This makes it possible for kernel to defaults to >>> relatime or noatime but still allow userspace to >>> override it. For more details about the default system >>> mount options see /proc/mounts. >>> >>> It's passed down to the kernel via the MS_STRICTATIME flag. All >>> you need to do is make ubifs aware of this flag... >> >> Hi Dave, thanx for your suggestiong, but sorry, it's a little confusing >> to me :(. > > I do not know the history, but IIUC, this is what Dave's hint translates > to for UBIFS: > > -o - default behavior (no atime) > -o atime - default behavior (no atime) > -o noatime - default behavior (no atime) > > -o strictatime - full atime support > -o relatime - relative atime support > -o lazyatime - lazy atime support > > Is this logical from user's perspective? No, but this is a standard > "hack", not an UBIFS-only "hack", so we are fine. > > "force_atime" that you are suggesting would be UBIFS-only hack, which is > not as fine as a standard and documented "hack". > > IOW, atime/noatime are the "don't use" options, they are ignored and > every file-system is free to use its own defaults, be that noatime or > relatime or strictatime. If you want to tell the FS what to do, use > strictatime/relatime/lazyatime. Ha, okey, I believe there was some misunderstanding between us. Yes, this is what we want: > -o - default behavior (no atime) > -o atime - default behavior (no atime) > -o noatime - default behavior (no atime) > > -o strictatime - full atime support > -o relatime - relative atime support > -o lazyatime - lazy atime support That's great!! But there is a problem to implement it. Because we can not distinguish the cases below: > -o - default behavior (no atime) > -o relatime - relative atime support We would find both of them are MS_RELATIME set. But we want to do different thing in these cases. So I introduced the force_atime. Then: -o - no atime support -o atime - no atime support -o noatime - same, no atime support -o force_atime - default behavior (relatime currently) -o force_atime,relatime - relative atime support -o force_atime,strictatime - strict atime support -o force_atime,lazyatime - lazy atime support But I agree that introducing a UBIFS_ATIME_SUPPORT as you suggested. Yang > > Does it make sense? > > . > -- 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
On Thu, 2015-06-25 at 18:10 +0800, Dongsheng Yang wrote: > > -o - default behavior (no atime) > > -o relatime - relative atime support > > We would find both of them are MS_RELATIME set. But we > want to do different thing in these cases. So I introduced > the force_atime. Then: Oh, do you know where exactly the default MS_RELATIME gets set? -- 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
On 06/25/2015 07:28 PM, Artem Bityutskiy wrote: > On Thu, 2015-06-25 at 18:10 +0800, Dongsheng Yang wrote: >> > -o - default behavior (no atime) >> > -o relatime - relative atime support >> >> We would find both of them are MS_RELATIME set. But we >> want to do different thing in these cases. So I introduced >> the force_atime. Then: > > Oh, do you know where exactly the default MS_RELATIME gets set? Ha, yes, it was set in do_mount() in vfs. I mentioned this in a mail days ago, but let me try to explain it more clearly here. Sorry for the looong mail :(. * The main idea here is to find a flexible way to make ubifs to support atime: * 1, To make atime supporting optional to user at first. * 2, To keep the compatibility currently. (a), the generic options in vfs: =================generic======================= -o - default behavior (relatime currently) -o atime - atime support -o noatime - no atime support -o relatime - relative atime support -o strictatime - strict atime support -o lazyatime - lazy atime support (b), My first idea about it is making ubifs support atime but keep the default to noatime. =================idea 1 in ubifs=============== -o - default behavior (*no atime*) <-----keep the default to *noatime* -o atime - atime support -o noatime - no atime support -o relatime - relative atime support -o strictatime - strict atime support -o lazyatime - lazy atime support But there are two problems to do it. (1), we can not distinguish them: -o - default behavior (*no atime*) -o relatime - relative atime support To solve it, I planed to introduce file_system_type::parse_options() then, file system can be in charge of the standard options. I am not sure is that acceptable to vfs guys, but it's possible way to solve it. (2), we can not distinguish them: -o - default behavior (*no atime*) -o atime - atime support This one is much difficult to solve. Because I found even in vfs, we can not know the difference. They are made to same in userspace by util-linux. Yes, we can solve it by introduce a MS_ATIME, but that's meaningless to others and we have to change code in user and kernel. So, it's unacceptable even to myself. (c), So, I dropped the *idea 1*. And find out an idea 2. =======================idea 2 in ubifs======================= -o - no atime -o atime - no atime -o noatime - no atime -o relatime - no atime -o strictatime - no atime -o lazyatime - no atime -o force_atime - default behavior (relatime currently) -o force_atime,atime - atime support -o force_atime,noatime - no atime support -o force_atime,relatime - relative atime support -o force_atime,strictatime - strict atime support -o force_atime,lazyatime - lazy atime support That means, we keep a *full compatibility* backward, and provide a force_atime option to make ubifs to work same with *generic* by *-o force_atime,...*. It's optional to user. force_atime works like a switch for atime supporting. (d), But when I heard an idea about UBIFS_ATIME_SUPPORT from you. I get an idea 3. ======================idea 3 in ubifs========================= UBIFS_ATIME_SUPPORT is n, same with what ubifs did: -o - no atime -o atime - no atime -o noatime - no atime -o relatime - no atime -o strictatime - no atime -o lazyatime - no atime UBIFS_ATIME_SUPPORT is y, same with what generic is doing: -o - default behavior (relatime currently) -o atime - atime support -o noatime - no atime support -o relatime - relative atime support -o strictatime - strict atime support -o lazyatime - lazy atime support I think this one is better than others, So I planed to do the *idea 3*. Thanx Yang > > . > -- 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
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c index 35efc10..e683890 100644 --- a/fs/ubifs/file.c +++ b/fs/ubifs/file.c @@ -1541,11 +1541,15 @@ static const struct vm_operations_struct ubifs_file_vm_ops = { static int ubifs_file_mmap(struct file *file, struct vm_area_struct *vma) { int err; + struct inode *inode = file->f_mapping->host; + struct ubifs_info *c = inode->i_sb->s_fs_info; err = generic_file_mmap(file, vma); if (err) return err; vma->vm_ops = &ubifs_file_vm_ops; + if (c->force_atime) + file_accessed(file); return 0; } diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index 75e6f04..8c2773b 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -128,7 +128,9 @@ struct inode *ubifs_iget(struct super_block *sb, unsigned long inum) if (err) goto out_ino; - inode->i_flags |= (S_NOCMTIME | S_NOATIME); + inode->i_flags |= S_NOCMTIME; + if (!c->force_atime) + inode->i_flags |= S_NOATIME; set_nlink(inode, le32_to_cpu(ino->nlink)); i_uid_write(inode, le32_to_cpu(ino->uid)); i_gid_write(inode, le32_to_cpu(ino->gid)); @@ -378,15 +380,47 @@ done: clear_inode(inode); } +/* + * There is only one possible caller of ubifs_dirty_inode without holding + * ui->ui_mutex, file_accessed. We are going to support atime if user + * set the mount option of force_atime. In that case, ubifs_dirty_inode + * need to lock ui->ui_mutex by itself and do a budget by itself. + */ static void ubifs_dirty_inode(struct inode *inode, int flags) { struct ubifs_inode *ui = ubifs_inode(inode); + int locked = mutex_is_locked(&ui->ui_mutex); + struct ubifs_info *c = inode->i_sb->s_fs_info; + int ret = 0; + + if (!locked) + mutex_lock(&ui->ui_mutex); - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); if (!ui->dirty) { + if (!locked) { + /* + * It's a little tricky here, there is only one + * possible user of ubifs_dirty_inode did not do + * a budget for this inode. At the same time, this + * user is not holding the ui->ui_mutex. Then if + * we found ui->ui_mutex is not locked, we can say: + * we need to do a budget in ubifs_dirty_inode here. + */ + struct ubifs_budget_req req = { .dirtied_ino = 1, + .dirtied_ino_d = ALIGN(ui->data_len, 8) }; + + ret = ubifs_budget_space(c, &req); + if (ret) + goto out; + } ui->dirty = 1; dbg_gen("inode %lu", inode->i_ino); } + +out: + if (!locked) + mutex_unlock(&ui->ui_mutex); + return; } static int ubifs_statfs(struct dentry *dentry, struct kstatfs *buf) @@ -440,6 +474,9 @@ static int ubifs_show_options(struct seq_file *s, struct dentry *root) ubifs_compr_name(c->mount_opts.compr_type)); } + if (c->mount_opts.force_atime == 1) + seq_printf(s, ",force_atime"); + return 0; } @@ -915,6 +952,7 @@ static int check_volume_empty(struct ubifs_info *c) * Opt_chk_data_crc: check CRCs when reading data nodes * Opt_no_chk_data_crc: do not check CRCs when reading data nodes * Opt_override_compr: override default compressor + * Opt_force_atime: enforce inode to support atime * Opt_err: just end of array marker */ enum { @@ -925,6 +963,7 @@ enum { Opt_chk_data_crc, Opt_no_chk_data_crc, Opt_override_compr, + Opt_force_atime, Opt_err, }; @@ -936,6 +975,7 @@ static const match_table_t tokens = { {Opt_chk_data_crc, "chk_data_crc"}, {Opt_no_chk_data_crc, "no_chk_data_crc"}, {Opt_override_compr, "compr=%s"}, + {Opt_force_atime, "force_atime"}, {Opt_err, NULL}, }; @@ -1036,6 +1076,10 @@ static int ubifs_parse_options(struct ubifs_info *c, char *options, c->default_compr = c->mount_opts.compr_type; break; } + case Opt_force_atime: + c->mount_opts.force_atime = 1; + c->force_atime = 1; + break; default: { unsigned long flag; @@ -2138,7 +2182,9 @@ static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags, if (err) goto out_deact; /* We do not support atime */ - sb->s_flags |= MS_ACTIVE | MS_NOATIME; + sb->s_flags |= MS_ACTIVE; + if (!c->force_atime) + sb->s_flags |= MS_NOATIME; } /* 'fill_super()' opens ubi again so we must close it here */ diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h index de75902..fbc46a2 100644 --- a/fs/ubifs/ubifs.h +++ b/fs/ubifs/ubifs.h @@ -942,6 +942,8 @@ struct ubifs_orphan { * specified in @compr_type) * @compr_type: compressor type to override the superblock compressor with * (%UBIFS_COMPR_NONE, etc) + * @force_atime: ubifs does not support atime by default, but if you set + * force_atime in mount opts, we update the atime accessing. */ struct ubifs_mount_opts { unsigned int unmount_mode:2; @@ -949,6 +951,7 @@ struct ubifs_mount_opts { unsigned int chk_data_crc:2; unsigned int override_compr:1; unsigned int compr_type:2; + unsigned int force_atime:1; }; /** @@ -1034,6 +1037,7 @@ struct ubifs_debug_info; * @bulk_read: enable bulk-reads * @default_compr: default compression algorithm (%UBIFS_COMPR_LZO, etc) * @rw_incompat: the media is not R/W compatible + * @force_atime: support atime if it is set * * @tnc_mutex: protects the Tree Node Cache (TNC), @zroot, @cnext, @enext, and * @calc_idx_sz @@ -1275,6 +1279,7 @@ struct ubifs_info { unsigned int bulk_read:1; unsigned int default_compr:2; unsigned int rw_incompat:1; + unsigned int force_atime:1; struct mutex tnc_mutex; struct ubifs_zbranch zroot;
Currently, ubifs does not support access time anyway. I understand that there is a overhead to update inode in each access from user. But for the following two reasons, I think we can make it optional to user. (1). More and more flash storage in server are trying to use ubifs, it is not only for a device such as mobile phone any more, we want to use it in more and more generic way. Then we need to compete with some other main filesystems. From this point, access time is necessary to us, at least as a choice to user currently. (2). The default mount option about atime is relatime currently, it's much relaxy compared with strictatime. Then we don't update the inode in any accessing. So the overhead is not too much. It's really acceptable. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- It's a RESEND patch to cc to fsdevel as Artem suggested. I would rename force_atime to enable_atime in next version. fs/ubifs/file.c | 4 ++++ fs/ubifs/super.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- fs/ubifs/ubifs.h | 5 +++++ 3 files changed, 58 insertions(+), 3 deletions(-)