xfs
[Top] [All Lists]

Re: [PATCH-v3 3/6] vfs: don't let the dirty time inodes get more than a

To: Theodore Ts'o <tytso@xxxxxxx>
Subject: Re: [PATCH-v3 3/6] vfs: don't let the dirty time inodes get more than a day stale
From: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
Date: Tue, 25 Nov 2014 15:58:01 +0100
Cc: Linux Filesystem Development List <linux-fsdevel@xxxxxxxxxxxxxxx>, Ext4 Developers List <linux-ext4@xxxxxxxxxxxxxxx>, Linux btrfs Developers List <linux-btrfs@xxxxxxxxxxxxxxx>, XFS Developers <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rasmusvillemoes.dk; s=google; h=from:to:cc:subject:organization:references:date:in-reply-to :message-id:user-agent:mime-version:content-type; bh=6e8xiUjJXhRxGqoVpm97XhpME/Dr5Bx8hLXTv5YMCW4=; b=OPy3X+/kJFzaOLFBfU8orI5kyCruo9s4sGUGlUPIorgLMZzQGOBraNgL46WMi5TI2o ZkEHDHk+UnOGlsYpUjuTuM95kAO4FdUweoCKZr7fBukLDh9u2ydFThvvf+VOapxRzq9Z iSMRWGDBhn1o/il0aQfRegb3uQKp5LErGvDtY=
In-reply-to: <1416893674-419-4-git-send-email-tytso@xxxxxxx> (Theodore Ts'o's message of "Tue, 25 Nov 2014 00:34:31 -0500")
Organization: D03
References: <1416893674-419-1-git-send-email-tytso@xxxxxxx> <1416893674-419-4-git-send-email-tytso@xxxxxxx>
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)
On Tue, Nov 25 2014, Theodore Ts'o <tytso@xxxxxxx> wrote:

>  static int update_time(struct inode *inode, struct timespec *time, int flags)
>  {
> +     struct timespec uptime;
> +     unsigned short daycode;
>       int ret;
>  
>       if (inode->i_op->update_time) {
> @@ -1525,17 +1527,33 @@ static int update_time(struct inode *inode, struct 
> timespec *time, int flags)
>               if (flags & S_CTIME)
>                       inode->i_ctime = *time;
>               if (flags & S_MTIME)
> -                     inode->i_mtime = *time;
> +             inode->i_mtime = *time;
>       }
> +     /*
> +      * If i_ts_dirty_day is zero, then either we have not deferred
> +      * timestamp updates, or the system has been up for less than
> +      * a day (so days_since_boot is zero), so we defer timestamp
> +      * updates in that case and set the I_DIRTY_TIME flag.  If a
> +      * day or more has passed, then i_ts_dirty_day will be
> +      * different from days_since_boot, and then we should update
> +      * the on-disk inode and then we can clear i_ts_dirty_day.
> +      */

I think days_since_boot was a lot clearer than daycode. In any case,
please make the comment and the code consistent.

>       if ((inode->i_sb->s_flags & MS_LAZYTIME) &&
>           !(flags & S_VERSION)) {
>               if (inode->i_state & I_DIRTY_TIME)
>                       return 0;
> -             spin_lock(&inode->i_lock);
> -             inode->i_state |= I_DIRTY_TIME;
> -             spin_unlock(&inode->i_lock);
> -             return 0;
> +             get_monotonic_boottime(&uptime);
> +             daycode = div_u64(uptime.tv_sec, (HZ * 86400));

You should probably divide by the number of seconds in a day, not the
number of jiffies in a day.

Isn't div_u64 mostly for when the divisor is not known at compile time?
Technically, "(u64)uptime.tv_sec / 86400" is of course a u64/u64 division,
but the compiler should see that the divisor is only 32 bits and hence
should be able to generate code which is at least as efficient as
whatever inline asm the arch provides for u64/u32 divisions.


Rasmus

<Prev in Thread] Current Thread [Next in Thread>