All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: "Theodore Ts'o" <tytso@mit.edu>
Cc: linux-fsdevel@vger.kernel.org,
	Ext4 Developers List <linux-ext4@vger.kernel.org>,
	xfs@oss.sgi.com, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH-v2 3/5] vfs: don't let the dirty time inodes get more than a day stale
Date: Mon, 24 Nov 2014 13:27:21 +0100	[thread overview]
Message-ID: <87egss3hsm.fsf@rasmusvillemoes.dk> (raw)
In-Reply-To: <1416675267-2191-4-git-send-email-tytso@mit.edu> (Theodore Ts'o's message of "Sat, 22 Nov 2014 11:54:25 -0500")

On Sat, Nov 22 2014, Theodore Ts'o <tytso@mit.edu> wrote:

> Guarantee that the on-disk timestamps will be no more than 24 hours
> stale.
>
>  static int update_time(struct inode *inode, struct timespec *time, int flags)
>  {
> +	unsigned short days_since_boot = jiffies / (HZ * 86400);
>  	int ret;
>

This seems to wrap every 49 days (assuming 32 bit jiffies and HZ==1000),
so on-disk updates can be delayed indefinitely, assuming just the right
delays between writes.

>  	if (inode->i_op->update_time) {
> @@ -1527,14 +1528,27 @@ static int update_time(struct inode *inode, struct timespec *time, int flags)
>  		if (flags & S_MTIME)
>  			inode->i_mtime = *time;
>  	}
> -	if (inode->i_sb->s_flags & MS_LAZYTIME) {
> +	/*
> +	 * 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.
> +	 */

AFAICT days_since_boot is not actually 0 immediately after boot
due to 

#define INITIAL_JIFFIES ((unsigned long)(unsigned int) (-300*HZ))

On 32 bit platforms, days_since_boot will be 0 shortly after, while on
64 bit it will always be >= 49. Not exactly sure how this affects the
above logic.

Would it make sense to introduce days_since_boot as a global variable
and avoid these issues? This would presumably also make update_time a
few cycles faster (avoiding a division-by-constant), but not sure if
that's important. And something of course needs to update
days_since_boot, but that should be doable.

Rasmus


WARNING: multiple messages have this Message-ID (diff)
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Theodore Ts'o <tytso@mit.edu>
Cc: linux-fsdevel@vger.kernel.org,
	Ext4 Developers List <linux-ext4@vger.kernel.org>,
	linux-btrfs@vger.kernel.org, xfs@oss.sgi.com
Subject: Re: [PATCH-v2 3/5] vfs: don't let the dirty time inodes get more than a day stale
Date: Mon, 24 Nov 2014 13:27:21 +0100	[thread overview]
Message-ID: <87egss3hsm.fsf@rasmusvillemoes.dk> (raw)
In-Reply-To: <1416675267-2191-4-git-send-email-tytso@mit.edu> (Theodore Ts'o's message of "Sat, 22 Nov 2014 11:54:25 -0500")

On Sat, Nov 22 2014, Theodore Ts'o <tytso@mit.edu> wrote:

> Guarantee that the on-disk timestamps will be no more than 24 hours
> stale.
>
>  static int update_time(struct inode *inode, struct timespec *time, int flags)
>  {
> +	unsigned short days_since_boot = jiffies / (HZ * 86400);
>  	int ret;
>

This seems to wrap every 49 days (assuming 32 bit jiffies and HZ==1000),
so on-disk updates can be delayed indefinitely, assuming just the right
delays between writes.

>  	if (inode->i_op->update_time) {
> @@ -1527,14 +1528,27 @@ static int update_time(struct inode *inode, struct timespec *time, int flags)
>  		if (flags & S_MTIME)
>  			inode->i_mtime = *time;
>  	}
> -	if (inode->i_sb->s_flags & MS_LAZYTIME) {
> +	/*
> +	 * 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.
> +	 */

AFAICT days_since_boot is not actually 0 immediately after boot
due to 

#define INITIAL_JIFFIES ((unsigned long)(unsigned int) (-300*HZ))

On 32 bit platforms, days_since_boot will be 0 shortly after, while on
64 bit it will always be >= 49. Not exactly sure how this affects the
above logic.

Would it make sense to introduce days_since_boot as a global variable
and avoid these issues? This would presumably also make update_time a
few cycles faster (avoiding a division-by-constant), but not sure if
that's important. And something of course needs to update
days_since_boot, but that should be doable.

Rasmus

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

  reply	other threads:[~2014-11-24 12:27 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-22 16:54 [PATCH-v2 0/5] add support for a lazytime mount option Theodore Ts'o
2014-11-22 16:54 ` Theodore Ts'o
2014-11-22 16:54 ` [PATCH-v2 1/5] fs: split update_time() into update_time() and write_time() Theodore Ts'o
2014-11-22 16:54   ` Theodore Ts'o
2014-11-22 16:54 ` [PATCH-v2 2/5] vfs: add support for a lazytime mount option Theodore Ts'o
2014-11-22 16:54   ` Theodore Ts'o
2014-11-22 16:54 ` [PATCH-v2 3/5] vfs: don't let the dirty time inodes get more than a day stale Theodore Ts'o
2014-11-22 16:54   ` Theodore Ts'o
2014-11-24 12:27   ` Rasmus Villemoes [this message]
2014-11-24 12:27     ` Rasmus Villemoes
2014-11-24 17:10     ` Theodore Ts'o
2014-11-24 17:10       ` Theodore Ts'o
2014-11-22 16:54 ` [PATCH-v2 4/5] vfs: add lazytime tracepoints for better debugging Theodore Ts'o
2014-11-22 16:54   ` Theodore Ts'o
2014-11-22 16:54 ` [PATCH-v2 5/5] ext4: add support for a lazytime mount option Theodore Ts'o
2014-11-22 16:54   ` Theodore Ts'o
2014-11-24  9:07 ` [PATCH-v2 0/5] " Christoph Hellwig
2014-11-24  9:07   ` Christoph Hellwig
2014-11-24 11:57   ` Theodore Ts'o
2014-11-24 11:57     ` Theodore Ts'o
2014-11-24 22:11     ` J. Bruce Fields
2014-11-24 22:11       ` J. Bruce Fields
2014-11-25  0:32       ` Theodore Ts'o
2014-11-25  0:32         ` Theodore Ts'o

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87egss3hsm.fsf@rasmusvillemoes.dk \
    --to=linux@rasmusvillemoes.dk \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=tytso@mit.edu \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.