[BUG] mtime&ctime updated when it should not

[BUG] mtime&ctime updated when it should not

[Jan Kara]

  Hello,

  one user pointed my attention to the fact that when the write fails
(for example when the user quota is exceeded) the modification time is
still updated (the problem appears both in 2.4 and 2.6). According to
SUSv3 that should not happen because the specification says that mtime
and ctime should be marked for update upon a successful completition
of a write (not that it would forbid updating the times in other cases
but I find it at least a bit nonintuitive).
  The easiest fix would be probably to "backup" the times at the
beginning of the write and restore the original values when the write
fails (simply not updating the times would require more surgery because
for example vmtruncate() is called when the write fails and it also
updates the times).
  So should I write the patch or is the current behaviour considered
correct?

								Honza

-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: [BUG] mtime&ctime updated when it should not

[Herbert Poetzl]

On Mon, Sep 01, 2003 at 08:11:13PM +0200, Jan Kara wrote:
>   Hello,
> 
>   one user pointed my attention to the fact that when the write fails
> (for example when the user quota is exceeded) the modification time is
> still updated (the problem appears both in 2.4 and 2.6). According to
> SUSv3 that should not happen because the specification says that mtime
> and ctime should be marked for update upon a successful completition
> of a write (not that it would forbid updating the times in other cases
> but I find it at least a bit nonintuitive).
>   The easiest fix would be probably to "backup" the times at the
> beginning of the write and restore the original values when the write
> fails (simply not updating the times would require more surgery because
> for example vmtruncate() is called when the write fails and it also
> updates the times).
>   So should I write the patch or is the current behaviour considered
> correct?

hmm, what if the request only partially succeeds?

for example echo "five" >/tmp/x will create /tmp/x
if inode limit permits it, but will leave it empty
if the space limit does not ...

personally I wouldn't care about the modification
time on such a quota fault ...

best,
Herbert

> 								Honza
> 
> -- 
> Jan Kara <jack@suse.cz>
> SuSE CR Labs

Re: [BUG] mtime&ctime updated when it should not

[Jan Kara]

> On Mon, Sep 01, 2003 at 08:11:13PM +0200, Jan Kara wrote:
> >   Hello,
> > 
> >   one user pointed my attention to the fact that when the write fails
> > (for example when the user quota is exceeded) the modification time is
> > still updated (the problem appears both in 2.4 and 2.6). According to
> > SUSv3 that should not happen because the specification says that mtime
> > and ctime should be marked for update upon a successful completition
> > of a write (not that it would forbid updating the times in other cases
> > but I find it at least a bit nonintuitive).
> >   The easiest fix would be probably to "backup" the times at the
> > beginning of the write and restore the original values when the write
> > fails (simply not updating the times would require more surgery because
> > for example vmtruncate() is called when the write fails and it also
> > updates the times).
> >   So should I write the patch or is the current behaviour considered
> > correct?
> 
> hmm, what if the request only partially succeeds?
> 
> for example echo "five" >/tmp/x will create /tmp/x
> if inode limit permits it, but will leave it empty
> if the space limit does not ...
  In thi case actually open(2) succeeds (so times will be set correctly)
but following write(2) fails so it won't change times...

> personally I wouldn't care about the modification
> time on such a quota fault ...
  In my opinion correct behaviour is to change times if at least one
byte is written...

								Honza

-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: [BUG] mtime&ctime updated when it should not

[Andrew Morton]

Jan Kara <jack@suse.cz> wrote:
>
>   Hello,
> 
>   one user pointed my attention to the fact that when the write fails
> (for example when the user quota is exceeded) the modification time is
> still updated (the problem appears both in 2.4 and 2.6). According to
> SUSv3 that should not happen because the specification says that mtime
> and ctime should be marked for update upon a successful completition
> of a write (not that it would forbid updating the times in other cases
> but I find it at least a bit nonintuitive).

hrm.  Doesn't sound super-important.  But..

>   The easiest fix would be probably to "backup" the times at the
> beginning of the write and restore the original values when the write
> fails (simply not updating the times would require more surgery because
> for example vmtruncate() is called when the write fails and it also
> updates the times).
>   So should I write the patch or is the current behaviour considered
> correct?

Isn't this sufficient?


diff -puN mm/filemap.c~a mm/filemap.c
--- 25/mm/filemap.c~a	2003-09-01 12:16:13.000000000 -0700
+++ 25-akpm/mm/filemap.c	2003-09-01 12:17:04.000000000 -0700
@@ -1696,7 +1696,6 @@ generic_file_aio_write_nolock(struct kio
 		goto out;
 
 	remove_suid(file->f_dentry);
-	inode_update_time(inode, 1);
 
 	/* coalesce the iovecs and go direct-to-BIO for O_DIRECT */
 	if (unlikely(file->f_flags & O_DIRECT)) {
@@ -1811,7 +1810,12 @@ generic_file_aio_write_nolock(struct kio
 	}
 	
 out_status:	
-	err = written ? written : status;
+	if (written) {
+		err = written;
+		inode_update_time(inode, 1);
+	} else {
+		err = status;
+	}
 out:
 	pagevec_lru_add(&lru_pvec);
 	current->backing_dev_info = 0;

_

Re: [BUG] mtime&ctime updated when it should not

[Jan Kara]

> Jan Kara <jack@suse.cz> wrote:
> >
> >   Hello,
> > 
> >   one user pointed my attention to the fact that when the write fails
> > (for example when the user quota is exceeded) the modification time is
> > still updated (the problem appears both in 2.4 and 2.6). According to
> > SUSv3 that should not happen because the specification says that mtime
> > and ctime should be marked for update upon a successful completition
> > of a write (not that it would forbid updating the times in other cases
> > but I find it at least a bit nonintuitive).
> 
> hrm.  Doesn't sound super-important.  But..
  I agree that it is a minor problem...

> >   The easiest fix would be probably to "backup" the times at the
> > beginning of the write and restore the original values when the write
> > fails (simply not updating the times would require more surgery because
> > for example vmtruncate() is called when the write fails and it also
> > updates the times).
> >   So should I write the patch or is the current behaviour considered
> > correct?
> 
> Isn't this sufficient?
  I think it is not (I tried exactly the same patch but it didn't work)
- the problem is that vmtruncate() is called when prepare_write() fails
and this function also updates mtime and ctime.

								Honza

Re: [BUG] mtime&ctime updated when it should not

[Andrew Morton]

Jan Kara <jack@suse.cz> wrote:
>
>  > Isn't this sufficient?
>    I think it is not (I tried exactly the same patch but it didn't work)
>  - the problem is that vmtruncate() is called when prepare_write() fails
>  and this function also updates mtime and ctime.

Oh OK.

So we would need to change each filesystem's ->truncate to not update the
inode times, then move the timestamp updating up into vmtruncate().

Re: [BUG] mtime&ctime updated when it should not

[Mike Fedyk]

On Mon, Sep 01, 2003 at 08:35:27PM +0200, Herbert Poetzl wrote:
> On Mon, Sep 01, 2003 at 08:11:13PM +0200, Jan Kara wrote:
> >   Hello,
> > 
> >   one user pointed my attention to the fact that when the write fails
> > (for example when the user quota is exceeded) the modification time is
> > still updated (the problem appears both in 2.4 and 2.6). According to
> > SUSv3 that should not happen because the specification says that mtime
> > and ctime should be marked for update upon a successful completition
> > of a write (not that it would forbid updating the times in other cases
> > but I find it at least a bit nonintuitive).
> >   The easiest fix would be probably to "backup" the times at the
> > beginning of the write and restore the original values when the write
> > fails (simply not updating the times would require more surgery because
> > for example vmtruncate() is called when the write fails and it also
> > updates the times).
> >   So should I write the patch or is the current behaviour considered
> > correct?
> 
> hmm, what if the request only partially succeeds?
> 
> for example echo "five" >/tmp/x will create /tmp/x
> if inode limit permits it, but will leave it empty
> if the space limit does not ...
> 
> personally I wouldn't care about the modification
> time on such a quota fault ...

At least you would know when the truncate happened, even if the write didn't
complete successfully.

Re: [BUG] mtime&ctime updated when it should not

[Jan Kara]

> Jan Kara <jack@suse.cz> wrote:
> >
> >  > Isn't this sufficient?
> >    I think it is not (I tried exactly the same patch but it didn't work)
> >  - the problem is that vmtruncate() is called when prepare_write() fails
> >  and this function also updates mtime and ctime.
> 
> Oh OK.
> 
> So we would need to change each filesystem's ->truncate to not update the
> inode times, then move the timestamp updating up into vmtruncate().
  That is one solution. The other (less intrusive) is to just store old
time stamps and restore times when you find out that write failed.
  I'll have a look how much work would be your solution..

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
> Jan Kara <jack@suse.cz> wrote:
> >
> >  > Isn't this sufficient?
> >    I think it is not (I tried exactly the same patch but it didn't work)
> >  - the problem is that vmtruncate() is called when prepare_write() fails
> >  and this function also updates mtime and ctime.
> 
> Oh OK.
> 
> So we would need to change each filesystem's ->truncate to not update the
> inode times, then move the timestamp updating up into vmtruncate().
  That is one solution. The other (less intrusive) is to just store old
time stamps and restore times when you find out that write failed.
  I'll have a look how much work would be your solution..

								Honza
-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: [BUG] mtime&ctime updated when it should not

[Pavel Machek]

Hi!

> > >  > Isn't this sufficient?
> > >    I think it is not (I tried exactly the same patch but it didn't work)
> > >  - the problem is that vmtruncate() is called when prepare_write() fails
> > >  and this function also updates mtime and ctime.
> > 
> > Oh OK.
> > 
> > So we would need to change each filesystem's ->truncate to not update the
> > inode times, then move the timestamp updating up into vmtruncate().
>   That is one solution. The other (less intrusive) is to just store old
> time stamps and restore times when you find out that write failed.

What if userspace sees the new time for a short while? That would certainly be
a bug...
				Pavel

-- 
				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...