Re: [PATCH v5] ubifs: introduce UBIFS_ATIME_SUPPORT to ubifs

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

On 08/18/2015 04:43 PM, Artem Bityutskiy wrote:
> Please, take a look at the mkwrite path, most of the file-systems update
> atime there too,
> I think your patch may need to do something about atime in
> 'ubifs_vm_page_mkwrite()'.

Hi Atem,
	most of the file-systems are updating the ctime and mtime
in mkwrite path by calling file_update_time().It's a writing path
so we don't update atime here.
>
> On Tue, Aug 18, 2015 at 7:40 AM, Dongsheng Yang
> <yangds.fnst@cn.fujitsu.com <mailto:yangds.fnst@cn.fujitsu.com>> wrote:
>
>     To make ubifs support atime flexily, this commit introduces
>     +config UBIFS_ATIME_SUPPORT
>     +       bool "Access time support" if UBIFS_FS
>     +       depends on UBIFS_FS
>     +       default n
>     +       help
>     +         This option allows ubifs to support atime. -o strictatime
>     is harmful to
>     +         your flash, we don't suggest it. But relatime and lazytime
>     are much
>     +         better.
>
>
> How about this.
>
> Originally UBIFS did not support atime, because it looked like a bad
> idea due
> increased flash wear. This option adds atime support and it is disabled
> by default
> to preserve the old behavior. If you enable this option, UBIFS starts
> updating atime,
> which means that file-system read operations will cause writes (inode atime
> updates). This may affect file-system performance and increase flash
> device wear,
> so be careful. How often atime is updated depends on the selected strategy:
> strictatime is the "heavy", relatime is "lighter", etc.

Much better :)
>
>
>     +int ubifs_update_time(struct inode *inode, struct timespec *time,
>     +                            int flags)
>     +{
>     +       struct ubifs_inode *ui = ubifs_inode(inode);
>     +       struct ubifs_info *c = inode->i_sb->s_fs_info;
>     +       struct ubifs_budget_req req = { .dirtied_ino = 1,
>     +                       .dirtied_ino_d = ALIGN(ui->data_len, 8) };
>     +       int iflags = I_DIRTY_TIME;
>     +       int err, release;
>     +
>     +       err = ubifs_budget_space(c, &req);
>     +       if (err)
>     +               return err;
>     +
>     +       mutex_lock(&ui->ui_mutex);
>
>     +       if (flags & S_ATIME)
>     +               inode->i_atime = *time;
>     +       if (flags & S_VERSION)
>     +               inode_inc_iversion(inode);
>     +       if (flags & S_CTIME)
>     +               inode->i_ctime = *time;
>     +       if (flags & S_MTIME)
>     +               inode->i_mtime = *time;
>     +
>     +       if (!(inode->i_sb->s_flags & MS_LAZYTIME) || (flags &
>     S_VERSION))
>     +               iflags |= I_DIRTY_SYNC;
>
>
> The lazytime part looks OK, but I am a bit concerned about the S_VERSION
> part.
> IIRC, the inode version stuff is needed for NFS, which UBIFS does not
> support
> anyway. I do not see that we do anything with inode version in UBIFS, so
> it looks
> like UBIFS just does not support this.
>
> I do not think we should add code we do not really need or understand.
> Could you
> please take a closer look to the S_VERSION stuff and either remove it
> from this
> patch or make sure UBIFS needs this piece of code, which I doubt.

Thanx, I looked some more about it.

Hmmm, yes, agree that we should not introduce some code here
to make it hard for understanding, although that would never
cause some problem. I will remove it.

Maybe we can reintroduce it when we plan to add nfs supporting
in ubifs.:)
>
>
>     -               sb->s_flags |= MS_ACTIVE | MS_NOATIME;
>     +               sb->s_flags |= MS_ACTIVE;
>     +#ifndef CONFIG_UBIFS_ATIME_SUPPORT
>     +               sb->s_flags |= MS_NOATIME;
>     +#else
>     +               ubifs_warn(c, "full atime support is enabled, which
>     may wear out your flash faster");
>     +#endif
>
>
> I am not sure we need to print this warning. If I know what I am doing,
> and want atime,
> why whould I see the warning?
>
> We made the default to be the old behavior, whoudn't it be enough to
> just print a
> message (ubifs_info()) about enabled atime, without the "which may wear
> out your flash
> faster" part, what do you think?

Hmmm "default behavior to disable" makes sense to me. Yes, if someone
  enable it by intention, we need not warning it out to him. Okey,
Just a ubifs_info sounds good to me.

Yang
>
> Thanks!
>
> --
> Best Regards,
> Artem Bityutskiy (Битюцкий Артём)