Re: [Cluster-devel] [f2fs-dev] [PATCH 01/13] iomap: update ki_pos a little later in iomap_dio_complete
Hello: This series was applied to jaegeuk/f2fs.git (dev) by Andrew Morton : On Fri, 19 May 2023 11:35:09 +0200 you wrote: > Move the ki_pos update down a bit to prepare for a better common > helper that invalidates pages based of an iocb. > > Signed-off-by: Christoph Hellwig > --- > fs/iomap/direct-io.c | 21 +++-- > 1 file changed, 11 insertions(+), 10 deletions(-) Here is the summary with links: - [f2fs-dev,01/13] iomap: update ki_pos a little later in iomap_dio_complete https://git.kernel.org/jaegeuk/f2fs/c/936e114a245b - [f2fs-dev,02/13] filemap: update ki_pos in generic_perform_write (no matching commit) - [f2fs-dev,03/13] filemap: assign current->backing_dev_info in generic_perform_write (no matching commit) - [f2fs-dev,04/13] filemap: add a kiocb_write_and_wait helper https://git.kernel.org/jaegeuk/f2fs/c/3c435a0fe35c - [f2fs-dev,05/13] filemap: add a kiocb_invalidate_pages helper https://git.kernel.org/jaegeuk/f2fs/c/e003f74afbd2 - [f2fs-dev,06/13] filemap: add a kiocb_invalidate_post_write helper (no matching commit) - [f2fs-dev,07/13] iomap: update ki_pos in iomap_file_buffered_write (no matching commit) - [f2fs-dev,08/13] iomap: assign current->backing_dev_info in iomap_file_buffered_write (no matching commit) - [f2fs-dev,09/13] iomap: use kiocb_write_and_wait and kiocb_invalidate_pages https://git.kernel.org/jaegeuk/f2fs/c/8ee93b4bb626 - [f2fs-dev,10/13] fs: factor out a direct_write_fallback helper (no matching commit) - [f2fs-dev,11/13] fuse: update ki_pos in fuse_perform_write (no matching commit) - [f2fs-dev,12/13] fuse: drop redundant arguments to fuse_perform_write (no matching commit) - [f2fs-dev,13/13] fuse: use direct_write_fallback (no matching commit) You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [Cluster-devel] [f2fs-dev] [PATCH 01/17] fs: unexport buffer_check_dirty_writeback
Hello: This series was applied to jaegeuk/f2fs.git (dev) by Jens Axboe : On Mon, 24 Apr 2023 07:49:10 +0200 you wrote: > buffer_check_dirty_writeback is only used by the block device aops, > remove the export. > > Signed-off-by: Christoph Hellwig > --- > fs/buffer.c | 1 - > 1 file changed, 1 deletion(-) Here is the summary with links: - [f2fs-dev,01/17] fs: unexport buffer_check_dirty_writeback https://git.kernel.org/jaegeuk/f2fs/c/4bb218a65a43 - [f2fs-dev,02/17] fs: remove the special !CONFIG_BLOCK def_blk_fops https://git.kernel.org/jaegeuk/f2fs/c/bda2795a630b - [f2fs-dev,03/17] fs: rename and move block_page_mkwrite_return (no matching commit) - [f2fs-dev,04/17] fs: remove emergency_thaw_bdev (no matching commit) - [f2fs-dev,05/17] filemap: update ki_pos in generic_perform_write (no matching commit) - [f2fs-dev,06/17] filemap: add a kiocb_write_and_wait helper https://git.kernel.org/jaegeuk/f2fs/c/3c435a0fe35c - [f2fs-dev,07/17] filemap: add a kiocb_invalidate_pages helper https://git.kernel.org/jaegeuk/f2fs/c/e003f74afbd2 - [f2fs-dev,08/17] filemap: add a kiocb_invalidate_post_write helper (no matching commit) - [f2fs-dev,09/17] fs: factor out a direct_write_fallback helper (no matching commit) - [f2fs-dev,10/17] iomap: use kiocb_write_and_wait and kiocb_invalidate_pages (no matching commit) - [f2fs-dev,11/17] iomap: assign current->backing_dev_info in iomap_file_buffered_write (no matching commit) - [f2fs-dev,12/17] fuse: use direct_write_fallback (no matching commit) - [f2fs-dev,13/17] block: don't plug in blkdev_write_iter https://git.kernel.org/jaegeuk/f2fs/c/712c7364655f - [f2fs-dev,14/17] block: open code __generic_file_write_iter for blkdev writes (no matching commit) - [f2fs-dev,15/17] block: stop setting ->direct_IO (no matching commit) - [f2fs-dev,16/17] block: use iomap for writes to block devices (no matching commit) - [f2fs-dev,17/17] fs: add CONFIG_BUFFER_HEAD (no matching commit) You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [Cluster-devel] [f2fs-dev] [PATCH 01/12] backing_dev: remove current->backing_dev_info
Hello:
This series was applied to jaegeuk/f2fs.git (dev)
by Andrew Morton :
On Thu, 1 Jun 2023 16:58:53 +0200 you wrote:
> The last user of current->backing_dev_info disappeared in commit
> b9b1335e6403 ("remove bdi_congested() and wb_congested() and related
> functions"). Remove the field and all assignments to it.
>
> Signed-off-by: Christoph Hellwig
> Reviewed-by: Christian Brauner
> Reviewed-by: Damien Le Moal
> Reviewed-by: Hannes Reinecke
> Reviewed-by: Johannes Thumshirn
> Reviewed-by: Darrick J. Wong
> Acked-by: Theodore Ts'o
>
> [...]
Here is the summary with links:
- [f2fs-dev,01/12] backing_dev: remove current->backing_dev_info
https://git.kernel.org/jaegeuk/f2fs/c/0d625446d0a4
- [f2fs-dev,02/12] iomap: update ki_pos a little later in iomap_dio_complete
https://git.kernel.org/jaegeuk/f2fs/c/936e114a245b
- [f2fs-dev,03/12] filemap: update ki_pos in generic_perform_write
https://git.kernel.org/jaegeuk/f2fs/c/182c25e9c157
- [f2fs-dev,04/12] filemap: add a kiocb_write_and_wait helper
https://git.kernel.org/jaegeuk/f2fs/c/3c435a0fe35c
- [f2fs-dev,05/12] filemap: add a kiocb_invalidate_pages helper
https://git.kernel.org/jaegeuk/f2fs/c/e003f74afbd2
- [f2fs-dev,06/12] filemap: add a kiocb_invalidate_post_direct_write helper
https://git.kernel.org/jaegeuk/f2fs/c/c402a9a9430b
- [f2fs-dev,07/12] iomap: update ki_pos in iomap_file_buffered_write
https://git.kernel.org/jaegeuk/f2fs/c/219580eea1ee
- [f2fs-dev,08/12] iomap: use kiocb_write_and_wait and kiocb_invalidate_pages
https://git.kernel.org/jaegeuk/f2fs/c/8ee93b4bb626
- [f2fs-dev,09/12] fs: factor out a direct_write_fallback helper
https://git.kernel.org/jaegeuk/f2fs/c/44fff0fa08ec
- [f2fs-dev,10/12] fuse: update ki_pos in fuse_perform_write
https://git.kernel.org/jaegeuk/f2fs/c/70e986c3b4f4
- [f2fs-dev,11/12] fuse: drop redundant arguments to fuse_perform_write
https://git.kernel.org/jaegeuk/f2fs/c/596df33d673d
- [f2fs-dev,12/12] fuse: use direct_write_fallback
https://git.kernel.org/jaegeuk/f2fs/c/64d1b4dd826d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [Cluster-devel] [PATCH v2 08/92] fs: new helper: simple_rename_timestamp
On Thu, 2023-07-06 at 08:19 +0900, Damien Le Moal wrote:
> On 7/6/23 03:58, Jeff Layton wrote:
> > A rename potentially involves updating 4 different inode timestamps. Add
> > a function that handles the details sanely, and convert the libfs.c
> > callers to use it.
> >
> > Signed-off-by: Jeff Layton
> > ---
> > fs/libfs.c | 36 +++-
> > include/linux/fs.h | 2 ++
> > 2 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index a7e56baf8bbd..9ee79668c909 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -692,6 +692,31 @@ int simple_rmdir(struct inode *dir, struct dentry
> > *dentry)
> > }
> > EXPORT_SYMBOL(simple_rmdir);
> >
> > +/**
> > + * simple_rename_timestamp - update the various inode timestamps for rename
> > + * @old_dir: old parent directory
> > + * @old_dentry: dentry that is being renamed
> > + * @new_dir: new parent directory
> > + * @new_dentry: target for rename
> > + *
> > + * POSIX mandates that the old and new parent directories have their ctime
> > and
> > + * mtime updated, and that inodes of @old_dentry and @new_dentry (if any),
> > have
> > + * their ctime updated.
> > + */
> > +void simple_rename_timestamp(struct inode *old_dir, struct dentry
> > *old_dentry,
> > +struct inode *new_dir, struct dentry *new_dentry)
> > +{
> > + struct inode *newino = d_inode(new_dentry);
> > +
> > + old_dir->i_mtime = inode_set_ctime_current(old_dir);
> > + if (new_dir != old_dir)
> > + new_dir->i_mtime = inode_set_ctime_current(new_dir);
> > + inode_set_ctime_current(d_inode(old_dentry));
> > + if (newino)
> > + inode_set_ctime_current(newino);
> > +}
> > +EXPORT_SYMBOL_GPL(simple_rename_timestamp);
> > +
> > int simple_rename_exchange(struct inode *old_dir, struct dentry
> > *old_dentry,
> >struct inode *new_dir, struct dentry *new_dentry)
> > {
> > @@ -707,11 +732,7 @@ int simple_rename_exchange(struct inode *old_dir,
> > struct dentry *old_dentry,
> > inc_nlink(old_dir);
> > }
> > }
> > - old_dir->i_ctime = old_dir->i_mtime =
> > - new_dir->i_ctime = new_dir->i_mtime =
> > - d_inode(old_dentry)->i_ctime =
> > - d_inode(new_dentry)->i_ctime = current_time(old_dir);
> > -
> > + simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
>
> This is somewhat changing the current behavior: before the patch, the mtime
> and
> ctime of old_dir, new_dir and the inodes associated with the dentries are
> always
> equal. But given that simple_rename_timestamp() calls
> inode_set_ctime_current()
> 4 times, the times could potentially be different.
>
> I am not sure if that is an issue, but it seems that calling
> inode_set_ctime_current() once, recording the "now" time it sets and using
> that
> value to set all times may be more efficient and preserve the existing
> behavior.
>
I don't believe it's an issue. I've seen nothing in the POSIX spec that
mandates that timestamp updates to different inodes involved in an
operation be set to the _same_ value. It just says they must be updated.
It's also hard to believe that any software would depend on this either,
given that it's very inconsistent across filesystems today. AFAICT, this
was mostly done in the past just as a matter of convenience.
The other problem with doing it that way is that it assumes that
current_time(inode) should always return the same value when given
different inodes. Is it really correct to do this?
inode_set_ctime(dir, inode_set_ctime_current(inode));
"dir" and "inode" are different inodes, after all, and you're setting
dir's timestamp to "inode"'s value. It's not a big deal today since
they're always on the same sb, but the ultimate goal of these changes is
to implement multigrain timestamps. That will mean that fetching a fine-
grained timestamp for an update when the existing mtime or ctime value
has been queried via getattr.
With that change, I think it's best that we treat updates to different
inodes individually, as some of them may require updating with a fine-
grained timestamp and some may not.
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(simple_rename_exchange);
> > @@ -720,7 +741,6 @@ int simple_rename(struct mnt_idmap *idmap, struct inode
> > *old_dir,
> > struct dentry *old_dentry, struct inode *new_dir,
> > struct dentry *new_dentry, unsigned int flags)
> > {
> > - struct inode *inode = d_inode(old_dentry);
> > int they_are_dirs = d_is_dir(old_dentry);
> >
> > if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
> > @@ -743,9 +763,7 @@ int simple_rename(struct mnt_idmap *idmap, struct inode
> > *old_dir,
> > inc_nlink(new_dir);
> > }
> >
> > - old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
> > - new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
> > -
> > +
Re: [Cluster-devel] [PATCH v2 92/92] fs: rename i_ctime field to __i_ctime
On 7/6/23 03:58, Jeff Layton wrote: > Now that everything in-tree is converted to use the accessor functions, > rename the i_ctime field in the inode to discourage direct access. > > Signed-off-by: Jeff Layton Looks OK to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [Cluster-devel] [PATCH 7/9] gfs2: update ctime when quota is updated
Am Mi., 5. Juli 2023 um 23:51 Uhr schrieb Jeff Layton : > > On Wed, 2023-07-05 at 22:25 +0200, Andreas Gruenbacher wrote: > > On Mon, Jun 12, 2023 at 12:36 PM Jeff Layton wrote: > > > On Fri, 2023-06-09 at 18:44 +0200, Andreas Gruenbacher wrote: > > > > Jeff, > > > > > > > > On Fri, Jun 9, 2023 at 2:50 PM Jeff Layton wrote: > > > > > Signed-off-by: Jeff Layton > > > > > --- > > > > > fs/gfs2/quota.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c > > > > > index 1ed17226d9ed..6d283e071b90 100644 > > > > > --- a/fs/gfs2/quota.c > > > > > +++ b/fs/gfs2/quota.c > > > > > @@ -869,7 +869,7 @@ static int gfs2_adjust_quota(struct gfs2_inode > > > > > *ip, loff_t loc, > > > > > size = loc + sizeof(struct gfs2_quota); > > > > > if (size > inode->i_size) > > > > > i_size_write(inode, size); > > > > > - inode->i_mtime = inode->i_atime = current_time(inode); > > > > > + inode->i_mtime = inode->i_atime = inode->i_ctime = > > > > > current_time(inode); > > > > > > > > I don't think we need to worry about the ctime of the quota inode as > > > > that inode is internal to the filesystem only. > > > > > > > > > > Thanks Andreas. I'll plan to drop this patch from the series for now. > > > > > > Does updating the mtime and atime here serve any purpose, or should > > > those also be removed? If you plan to keep the a/mtime updates then I'd > > > still suggest updating the ctime for consistency's sake. It shouldn't > > > cost anything extra to do so since you're dirtying the inode below > > > anyway. > > > > Yes, good point actually, we should keep things consistent for simplicity. > > > > Would you add this back in if you do another posting? > > > > I just re-posted the other patches in this as part of the ctime accessor > conversion. If I post again though, I can resurrect the gfs2 patch. If > not, we can do a follow-on fix later. Sure, not a big deal. > Since we're discussing it, it may be more correct to remove the atime > update there. gfs2_adjust_quota sounds like a "modify" operation, not a > "read", so I don't see a reason to update the atime. > > In general, the only time you only want to set the atime, ctime and > mtime in lockstep is when the inode is brand new. Yes, that makes sense, too. Thanks, Andreas
Re: [Cluster-devel] [PATCH v2 08/92] fs: new helper: simple_rename_timestamp
On 7/6/23 03:58, Jeff Layton wrote:
> A rename potentially involves updating 4 different inode timestamps. Add
> a function that handles the details sanely, and convert the libfs.c
> callers to use it.
>
> Signed-off-by: Jeff Layton
> ---
> fs/libfs.c | 36 +++-
> include/linux/fs.h | 2 ++
> 2 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/fs/libfs.c b/fs/libfs.c
> index a7e56baf8bbd..9ee79668c909 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -692,6 +692,31 @@ int simple_rmdir(struct inode *dir, struct dentry
> *dentry)
> }
> EXPORT_SYMBOL(simple_rmdir);
>
> +/**
> + * simple_rename_timestamp - update the various inode timestamps for rename
> + * @old_dir: old parent directory
> + * @old_dentry: dentry that is being renamed
> + * @new_dir: new parent directory
> + * @new_dentry: target for rename
> + *
> + * POSIX mandates that the old and new parent directories have their ctime
> and
> + * mtime updated, and that inodes of @old_dentry and @new_dentry (if any),
> have
> + * their ctime updated.
> + */
> +void simple_rename_timestamp(struct inode *old_dir, struct dentry
> *old_dentry,
> + struct inode *new_dir, struct dentry *new_dentry)
> +{
> + struct inode *newino = d_inode(new_dentry);
> +
> + old_dir->i_mtime = inode_set_ctime_current(old_dir);
> + if (new_dir != old_dir)
> + new_dir->i_mtime = inode_set_ctime_current(new_dir);
> + inode_set_ctime_current(d_inode(old_dentry));
> + if (newino)
> + inode_set_ctime_current(newino);
> +}
> +EXPORT_SYMBOL_GPL(simple_rename_timestamp);
> +
> int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
> struct inode *new_dir, struct dentry *new_dentry)
> {
> @@ -707,11 +732,7 @@ int simple_rename_exchange(struct inode *old_dir, struct
> dentry *old_dentry,
> inc_nlink(old_dir);
> }
> }
> - old_dir->i_ctime = old_dir->i_mtime =
> - new_dir->i_ctime = new_dir->i_mtime =
> - d_inode(old_dentry)->i_ctime =
> - d_inode(new_dentry)->i_ctime = current_time(old_dir);
> -
> + simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
This is somewhat changing the current behavior: before the patch, the mtime and
ctime of old_dir, new_dir and the inodes associated with the dentries are always
equal. But given that simple_rename_timestamp() calls inode_set_ctime_current()
4 times, the times could potentially be different.
I am not sure if that is an issue, but it seems that calling
inode_set_ctime_current() once, recording the "now" time it sets and using that
value to set all times may be more efficient and preserve the existing behavior.
> return 0;
> }
> EXPORT_SYMBOL_GPL(simple_rename_exchange);
> @@ -720,7 +741,6 @@ int simple_rename(struct mnt_idmap *idmap, struct inode
> *old_dir,
> struct dentry *old_dentry, struct inode *new_dir,
> struct dentry *new_dentry, unsigned int flags)
> {
> - struct inode *inode = d_inode(old_dentry);
> int they_are_dirs = d_is_dir(old_dentry);
>
> if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
> @@ -743,9 +763,7 @@ int simple_rename(struct mnt_idmap *idmap, struct inode
> *old_dir,
> inc_nlink(new_dir);
> }
>
> - old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
> - new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
> -
> + simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
> return 0;
> }
> EXPORT_SYMBOL(simple_rename);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index bdfbd11a5811..14e38bd900f1 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2979,6 +2979,8 @@ extern int simple_open(struct inode *inode, struct file
> *file);
> extern int simple_link(struct dentry *, struct inode *, struct dentry *);
> extern int simple_unlink(struct inode *, struct dentry *);
> extern int simple_rmdir(struct inode *, struct dentry *);
> +void simple_rename_timestamp(struct inode *old_dir, struct dentry
> *old_dentry,
> + struct inode *new_dir, struct dentry *new_dentry);
> extern int simple_rename_exchange(struct inode *old_dir, struct dentry
> *old_dentry,
> struct inode *new_dir, struct dentry
> *new_dentry);
> extern int simple_rename(struct mnt_idmap *, struct inode *,
--
Damien Le Moal
Western Digital Research
Re: [Cluster-devel] [PATCH v2 07/92] fs: add ctime accessors infrastructure
On 7/6/23 03:58, Jeff Layton wrote: > struct timespec64 has unused bits in the tv_nsec field that can be used > for other purposes. In future patches, we're going to change how the > inode->i_ctime is accessed in certain inodes in order to make use of > them. In order to do that safely though, we'll need to eradicate raw > accesses of the inode->i_ctime field from the kernel. > > Add new accessor functions for the ctime that we use to replace them. > > Reviewed-by: Jan Kara > Reviewed-by: Luis Chamberlain > Signed-off-by: Jeff Layton Looks OK to me. Reviewed-by: Damien Le Moal -- Damien Le Moal Western Digital Research
Re: [Cluster-devel] [PATCH v2 00/89] fs: new accessors for inode->i_ctime
On Wed, 2023-07-05 at 14:58 -0400, Jeff Layton wrote:
> v2:
> - prepend patches to add missing ctime updates
> - add simple_rename_timestamp helper function
> - rename ctime accessor functions as inode_get_ctime/inode_set_ctime_*
> - drop individual inode_ctime_set_{sec,nsec} helpers
>
> I've been working on a patchset to change how the inode->i_ctime is
> accessed in order to give us conditional, high-res timestamps for the
> ctime and mtime. struct timespec64 has unused bits in it that we can use
> to implement this. In order to do that however, we need to wrap all
> accesses of inode->i_ctime to ensure that bits used as flags are
> appropriately handled.
>
> The patchset starts with reposts of some missing ctime updates that I
> spotted in the tree. It then adds a new helper function for updating the
> timestamp after a successful rename, and new ctime accessor
> infrastructure.
>
> The bulk of the patchset is individual conversions of different
> subsysteme to use the new infrastructure. Finally, the patchset renames
> the i_ctime field to __i_ctime to help ensure that I didn't miss
> anything.
>
> This should apply cleanly to linux-next as of this morning.
>
> Most of this conversion was done via 5 different coccinelle scripts, run
> in succession, with a large swath of by-hand conversions to clean up the
> remainder.
>
A couple of other things I should note:
If you sent me an Acked-by or Reviewed-by in the previous set, then I
tried to keep it on the patch here, since the respun patches are mostly
just renaming stuff from v1. Let me know if I've missed any.
I've also pushed the pile to my tree as this tag:
https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git/tag/?h=ctime.20230705
In case that's easier to work with.
Cheers,
--
Jeff Layton
Re: [Cluster-devel] [PATCH 7/9] gfs2: update ctime when quota is updated
On Wed, 2023-07-05 at 22:25 +0200, Andreas Gruenbacher wrote: > On Mon, Jun 12, 2023 at 12:36 PM Jeff Layton wrote: > > On Fri, 2023-06-09 at 18:44 +0200, Andreas Gruenbacher wrote: > > > Jeff, > > > > > > On Fri, Jun 9, 2023 at 2:50 PM Jeff Layton wrote: > > > > Signed-off-by: Jeff Layton > > > > --- > > > > fs/gfs2/quota.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c > > > > index 1ed17226d9ed..6d283e071b90 100644 > > > > --- a/fs/gfs2/quota.c > > > > +++ b/fs/gfs2/quota.c > > > > @@ -869,7 +869,7 @@ static int gfs2_adjust_quota(struct gfs2_inode *ip, > > > > loff_t loc, > > > > size = loc + sizeof(struct gfs2_quota); > > > > if (size > inode->i_size) > > > > i_size_write(inode, size); > > > > - inode->i_mtime = inode->i_atime = current_time(inode); > > > > + inode->i_mtime = inode->i_atime = inode->i_ctime = > > > > current_time(inode); > > > > > > I don't think we need to worry about the ctime of the quota inode as > > > that inode is internal to the filesystem only. > > > > > > > Thanks Andreas. I'll plan to drop this patch from the series for now. > > > > Does updating the mtime and atime here serve any purpose, or should > > those also be removed? If you plan to keep the a/mtime updates then I'd > > still suggest updating the ctime for consistency's sake. It shouldn't > > cost anything extra to do so since you're dirtying the inode below > > anyway. > > Yes, good point actually, we should keep things consistent for simplicity. > > Would you add this back in if you do another posting? > I just re-posted the other patches in this as part of the ctime accessor conversion. If I post again though, I can resurrect the gfs2 patch. If not, we can do a follow-on fix later. Since we're discussing it, it may be more correct to remove the atime update there. gfs2_adjust_quota sounds like a "modify" operation, not a "read", so I don't see a reason to update the atime. In general, the only time you only want to set the atime, ctime and mtime in lockstep is when the inode is brand new. -- Jeff Layton
Re: [Cluster-devel] [PATCH 7/9] gfs2: update ctime when quota is updated
On Mon, Jun 12, 2023 at 12:36 PM Jeff Layton wrote: > On Fri, 2023-06-09 at 18:44 +0200, Andreas Gruenbacher wrote: > > Jeff, > > > > On Fri, Jun 9, 2023 at 2:50 PM Jeff Layton wrote: > > > Signed-off-by: Jeff Layton > > > --- > > > fs/gfs2/quota.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c > > > index 1ed17226d9ed..6d283e071b90 100644 > > > --- a/fs/gfs2/quota.c > > > +++ b/fs/gfs2/quota.c > > > @@ -869,7 +869,7 @@ static int gfs2_adjust_quota(struct gfs2_inode *ip, > > > loff_t loc, > > > size = loc + sizeof(struct gfs2_quota); > > > if (size > inode->i_size) > > > i_size_write(inode, size); > > > - inode->i_mtime = inode->i_atime = current_time(inode); > > > + inode->i_mtime = inode->i_atime = inode->i_ctime = > > > current_time(inode); > > > > I don't think we need to worry about the ctime of the quota inode as > > that inode is internal to the filesystem only. > > > > Thanks Andreas. I'll plan to drop this patch from the series for now. > > Does updating the mtime and atime here serve any purpose, or should > those also be removed? If you plan to keep the a/mtime updates then I'd > still suggest updating the ctime for consistency's sake. It shouldn't > cost anything extra to do so since you're dirtying the inode below > anyway. Yes, good point actually, we should keep things consistent for simplicity. Would you add this back in if you do another posting? Thanks, Andreas > Thanks! > > > > mark_inode_dirty(inode); > > > set_bit(QDF_REFRESH, >qd_flags); > > > } > > > -- > > > 2.40.1 > > > > > > > Thanks, > > Andreas > > > > -- > Jeff Layton >
[Cluster-devel] [PATCH v2 47/92] gfs2: convert to ctime accessor functions
In later patches, we're going to change how the inode's ctime field is
used. Switch to using accessor functions instead of raw accesses of
inode->i_ctime.
Signed-off-by: Jeff Layton
---
fs/gfs2/acl.c | 2 +-
fs/gfs2/bmap.c | 11 +--
fs/gfs2/dir.c | 15 ---
fs/gfs2/file.c | 2 +-
fs/gfs2/glops.c | 4 ++--
fs/gfs2/inode.c | 8
fs/gfs2/super.c | 4 ++--
fs/gfs2/xattr.c | 8
8 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
index a392aa0f041d..443640e6fb9c 100644
--- a/fs/gfs2/acl.c
+++ b/fs/gfs2/acl.c
@@ -142,7 +142,7 @@ int gfs2_set_acl(struct mnt_idmap *idmap, struct dentry
*dentry,
ret = __gfs2_set_acl(inode, acl, type);
if (!ret && mode != inode->i_mode) {
- inode->i_ctime = current_time(inode);
+ inode_set_ctime_current(inode);
inode->i_mode = mode;
mark_inode_dirty(inode);
}
diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
index 8d611fbcf0bd..45ea63f7167d 100644
--- a/fs/gfs2/bmap.c
+++ b/fs/gfs2/bmap.c
@@ -1386,7 +1386,7 @@ static int trunc_start(struct inode *inode, u64 newsize)
ip->i_diskflags |= GFS2_DIF_TRUNC_IN_PROG;
i_size_write(inode, newsize);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(>i_inode);
+ ip->i_inode.i_mtime = inode_set_ctime_current(>i_inode);
gfs2_dinode_out(ip, dibh->b_data);
if (journaled)
@@ -1583,8 +1583,7 @@ static int sweep_bh_for_rgrps(struct gfs2_inode *ip,
struct gfs2_holder *rd_gh,
/* Every transaction boundary, we rewrite the dinode
to keep its di_blocks current in case of failure. */
- ip->i_inode.i_mtime = ip->i_inode.i_ctime =
- current_time(>i_inode);
+ ip->i_inode.i_mtime =
inode_set_ctime_current(>i_inode);
gfs2_trans_add_meta(ip->i_gl, dibh);
gfs2_dinode_out(ip, dibh->b_data);
brelse(dibh);
@@ -1950,7 +1949,7 @@ static int punch_hole(struct gfs2_inode *ip, u64 offset,
u64 length)
gfs2_statfs_change(sdp, 0, +btotal, 0);
gfs2_quota_change(ip, -(s64)btotal, ip->i_inode.i_uid,
ip->i_inode.i_gid);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime =
current_time(>i_inode);
+ ip->i_inode.i_mtime = inode_set_ctime_current(>i_inode);
gfs2_trans_add_meta(ip->i_gl, dibh);
gfs2_dinode_out(ip, dibh->b_data);
up_write(>i_rw_mutex);
@@ -1993,7 +1992,7 @@ static int trunc_end(struct gfs2_inode *ip)
gfs2_buffer_clear_tail(dibh, sizeof(struct gfs2_dinode));
gfs2_ordered_del_inode(ip);
}
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(>i_inode);
+ ip->i_inode.i_mtime = inode_set_ctime_current(>i_inode);
ip->i_diskflags &= ~GFS2_DIF_TRUNC_IN_PROG;
gfs2_trans_add_meta(ip->i_gl, dibh);
@@ -2094,7 +2093,7 @@ static int do_grow(struct inode *inode, u64 size)
goto do_end_trans;
truncate_setsize(inode, size);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(>i_inode);
+ ip->i_inode.i_mtime = inode_set_ctime_current(>i_inode);
gfs2_trans_add_meta(ip->i_gl, dibh);
gfs2_dinode_out(ip, dibh->b_data);
brelse(dibh);
diff --git a/fs/gfs2/dir.c b/fs/gfs2/dir.c
index 54a6d17b8c25..1a2afa88f8be 100644
--- a/fs/gfs2/dir.c
+++ b/fs/gfs2/dir.c
@@ -130,7 +130,7 @@ static int gfs2_dir_write_stuffed(struct gfs2_inode *ip,
const char *buf,
memcpy(dibh->b_data + offset + sizeof(struct gfs2_dinode), buf, size);
if (ip->i_inode.i_size < offset + size)
i_size_write(>i_inode, offset + size);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(>i_inode);
+ ip->i_inode.i_mtime = inode_set_ctime_current(>i_inode);
gfs2_dinode_out(ip, dibh->b_data);
brelse(dibh);
@@ -227,7 +227,7 @@ static int gfs2_dir_write_data(struct gfs2_inode *ip, const
char *buf,
if (ip->i_inode.i_size < offset + copied)
i_size_write(>i_inode, offset + copied);
- ip->i_inode.i_mtime = ip->i_inode.i_ctime = current_time(>i_inode);
+ ip->i_inode.i_mtime = inode_set_ctime_current(>i_inode);
gfs2_trans_add_meta(ip->i_gl, dibh);
gfs2_dinode_out(ip, dibh->b_data);
@@ -1814,7 +1814,7 @@ int gfs2_dir_add(struct inode *inode, const struct qstr
*name,
gfs2_inum_out(nip, dent);
dent->de_type = cpu_to_be16(IF2DT(nip->i_inode.i_mode));
dent->de_rahead = cpu_to_be16(gfs2_inode_ra_len(nip));
- tv = current_time(>i_inode);
+ tv = inode_set_ctime_current(>i_inode);
[Cluster-devel] [PATCH v2 92/92] fs: rename i_ctime field to __i_ctime
Now that everything in-tree is converted to use the accessor functions,
rename the i_ctime field in the inode to discourage direct access.
Signed-off-by: Jeff Layton
---
include/linux/fs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 14e38bd900f1..b66442f91835 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -642,7 +642,7 @@ struct inode {
loff_t i_size;
struct timespec64 i_atime;
struct timespec64 i_mtime;
- struct timespec64 i_ctime;
+ struct timespec64 __i_ctime; /* use inode_*_ctime accessors! */
spinlock_t i_lock; /* i_blocks, i_bytes, maybe i_size */
unsigned short i_bytes;
u8 i_blkbits;
@@ -1485,7 +1485,7 @@ struct timespec64 inode_set_ctime_current(struct inode
*inode);
*/
static inline struct timespec64 inode_get_ctime(const struct inode *inode)
{
- return inode->i_ctime;
+ return inode->__i_ctime;
}
/**
@@ -1498,7 +1498,7 @@ static inline struct timespec64 inode_get_ctime(const
struct inode *inode)
static inline struct timespec64 inode_set_ctime_to_ts(struct inode *inode,
struct timespec64 ts)
{
- inode->i_ctime = ts;
+ inode->__i_ctime = ts;
return ts;
}
--
2.41.0
[Cluster-devel] [PATCH v2 08/92] fs: new helper: simple_rename_timestamp
A rename potentially involves updating 4 different inode timestamps. Add
a function that handles the details sanely, and convert the libfs.c
callers to use it.
Signed-off-by: Jeff Layton
---
fs/libfs.c | 36 +++-
include/linux/fs.h | 2 ++
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/fs/libfs.c b/fs/libfs.c
index a7e56baf8bbd..9ee79668c909 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -692,6 +692,31 @@ int simple_rmdir(struct inode *dir, struct dentry *dentry)
}
EXPORT_SYMBOL(simple_rmdir);
+/**
+ * simple_rename_timestamp - update the various inode timestamps for rename
+ * @old_dir: old parent directory
+ * @old_dentry: dentry that is being renamed
+ * @new_dir: new parent directory
+ * @new_dentry: target for rename
+ *
+ * POSIX mandates that the old and new parent directories have their ctime and
+ * mtime updated, and that inodes of @old_dentry and @new_dentry (if any), have
+ * their ctime updated.
+ */
+void simple_rename_timestamp(struct inode *old_dir, struct dentry *old_dentry,
+struct inode *new_dir, struct dentry *new_dentry)
+{
+ struct inode *newino = d_inode(new_dentry);
+
+ old_dir->i_mtime = inode_set_ctime_current(old_dir);
+ if (new_dir != old_dir)
+ new_dir->i_mtime = inode_set_ctime_current(new_dir);
+ inode_set_ctime_current(d_inode(old_dentry));
+ if (newino)
+ inode_set_ctime_current(newino);
+}
+EXPORT_SYMBOL_GPL(simple_rename_timestamp);
+
int simple_rename_exchange(struct inode *old_dir, struct dentry *old_dentry,
struct inode *new_dir, struct dentry *new_dentry)
{
@@ -707,11 +732,7 @@ int simple_rename_exchange(struct inode *old_dir, struct
dentry *old_dentry,
inc_nlink(old_dir);
}
}
- old_dir->i_ctime = old_dir->i_mtime =
- new_dir->i_ctime = new_dir->i_mtime =
- d_inode(old_dentry)->i_ctime =
- d_inode(new_dentry)->i_ctime = current_time(old_dir);
-
+ simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
return 0;
}
EXPORT_SYMBOL_GPL(simple_rename_exchange);
@@ -720,7 +741,6 @@ int simple_rename(struct mnt_idmap *idmap, struct inode
*old_dir,
struct dentry *old_dentry, struct inode *new_dir,
struct dentry *new_dentry, unsigned int flags)
{
- struct inode *inode = d_inode(old_dentry);
int they_are_dirs = d_is_dir(old_dentry);
if (flags & ~(RENAME_NOREPLACE | RENAME_EXCHANGE))
@@ -743,9 +763,7 @@ int simple_rename(struct mnt_idmap *idmap, struct inode
*old_dir,
inc_nlink(new_dir);
}
- old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
- new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
-
+ simple_rename_timestamp(old_dir, old_dentry, new_dir, new_dentry);
return 0;
}
EXPORT_SYMBOL(simple_rename);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index bdfbd11a5811..14e38bd900f1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2979,6 +2979,8 @@ extern int simple_open(struct inode *inode, struct file
*file);
extern int simple_link(struct dentry *, struct inode *, struct dentry *);
extern int simple_unlink(struct inode *, struct dentry *);
extern int simple_rmdir(struct inode *, struct dentry *);
+void simple_rename_timestamp(struct inode *old_dir, struct dentry *old_dentry,
+struct inode *new_dir, struct dentry *new_dentry);
extern int simple_rename_exchange(struct inode *old_dir, struct dentry
*old_dentry,
struct inode *new_dir, struct dentry
*new_dentry);
extern int simple_rename(struct mnt_idmap *, struct inode *,
--
2.41.0
[Cluster-devel] [PATCH v2 07/92] fs: add ctime accessors infrastructure
struct timespec64 has unused bits in the tv_nsec field that can be used
for other purposes. In future patches, we're going to change how the
inode->i_ctime is accessed in certain inodes in order to make use of
them. In order to do that safely though, we'll need to eradicate raw
accesses of the inode->i_ctime field from the kernel.
Add new accessor functions for the ctime that we use to replace them.
Reviewed-by: Jan Kara
Reviewed-by: Luis Chamberlain
Signed-off-by: Jeff Layton
---
fs/inode.c | 16
include/linux/fs.h | 45 -
2 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/fs/inode.c b/fs/inode.c
index d37fad91c8da..21b026d95b51 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2499,6 +2499,22 @@ struct timespec64 current_time(struct inode *inode)
}
EXPORT_SYMBOL(current_time);
+/**
+ * inode_set_ctime_current - set the ctime to current_time
+ * @inode: inode
+ *
+ * Set the inode->i_ctime to the current value for the inode. Returns
+ * the current value that was assigned to i_ctime.
+ */
+struct timespec64 inode_set_ctime_current(struct inode *inode)
+{
+ struct timespec64 now = current_time(inode);
+
+ inode_set_ctime(inode, now.tv_sec, now.tv_nsec);
+ return now;
+}
+EXPORT_SYMBOL(inode_set_ctime_current);
+
/**
* in_group_or_capable - check whether caller is CAP_FSETID privileged
* @idmap: idmap of the mount @inode was found from
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 824accb89a91..bdfbd11a5811 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1474,7 +1474,50 @@ static inline bool fsuidgid_has_mapping(struct
super_block *sb,
kgid_has_mapping(fs_userns, kgid);
}
-extern struct timespec64 current_time(struct inode *inode);
+struct timespec64 current_time(struct inode *inode);
+struct timespec64 inode_set_ctime_current(struct inode *inode);
+
+/**
+ * inode_get_ctime - fetch the current ctime from the inode
+ * @inode: inode from which to fetch ctime
+ *
+ * Grab the current ctime from the inode and return it.
+ */
+static inline struct timespec64 inode_get_ctime(const struct inode *inode)
+{
+ return inode->i_ctime;
+}
+
+/**
+ * inode_set_ctime_to_ts - set the ctime in the inode
+ * @inode: inode in which to set the ctime
+ * @ts: value to set in the ctime field
+ *
+ * Set the ctime in @inode to @ts
+ */
+static inline struct timespec64 inode_set_ctime_to_ts(struct inode *inode,
+ struct timespec64 ts)
+{
+ inode->i_ctime = ts;
+ return ts;
+}
+
+/**
+ * inode_set_ctime - set the ctime in the inode
+ * @inode: inode in which to set the ctime
+ * @sec: tv_sec value to set
+ * @nsec: tv_nsec value to set
+ *
+ * Set the ctime in @inode to { @sec, @nsec }
+ */
+static inline struct timespec64 inode_set_ctime(struct inode *inode,
+ time64_t sec, long nsec)
+{
+ struct timespec64 ts = { .tv_sec = sec,
+.tv_nsec = nsec };
+
+ return inode_set_ctime_to_ts(inode, ts);
+}
/*
* Snapshotting support.
--
2.41.0
[Cluster-devel] [PATCH v2 00/89] fs: new accessors for inode->i_ctime
v2:
- prepend patches to add missing ctime updates
- add simple_rename_timestamp helper function
- rename ctime accessor functions as inode_get_ctime/inode_set_ctime_*
- drop individual inode_ctime_set_{sec,nsec} helpers
I've been working on a patchset to change how the inode->i_ctime is
accessed in order to give us conditional, high-res timestamps for the
ctime and mtime. struct timespec64 has unused bits in it that we can use
to implement this. In order to do that however, we need to wrap all
accesses of inode->i_ctime to ensure that bits used as flags are
appropriately handled.
The patchset starts with reposts of some missing ctime updates that I
spotted in the tree. It then adds a new helper function for updating the
timestamp after a successful rename, and new ctime accessor
infrastructure.
The bulk of the patchset is individual conversions of different
subsysteme to use the new infrastructure. Finally, the patchset renames
the i_ctime field to __i_ctime to help ensure that I didn't miss
anything.
This should apply cleanly to linux-next as of this morning.
Most of this conversion was done via 5 different coccinelle scripts, run
in succession, with a large swath of by-hand conversions to clean up the
remainder.
The coccinelle scripts that were used are below:
::
cocci/ctime1.cocci
::
// convert as much to use inode_set_ctime_current as possible
@@
identifier timei;
struct inode *inode;
expression E1, E2;
@@
(
- inode->i_ctime = E1 = E2 = current_time(timei)
+ E1 = E2 = inode_set_ctime_current(inode)
|
- inode->i_ctime = E1 = current_time(timei)
+ E1 = inode_set_ctime_current(inode)
|
- E1 = inode->i_ctime = current_time(timei)
+ E1 = inode_set_ctime_current(inode)
|
- inode->i_ctime = current_time(timei)
+ inode_set_ctime_current(inode)
)
@@
struct inode *inode;
expression E1, E2, E3;
@@
(
- E1 = current_time(inode)
+ E1 = inode_set_ctime_current(inode)
|
- E1 = current_time(E3)
+ E1 = inode_set_ctime_current(inode)
)
...
(
- inode->i_ctime = E1;
|
- E2 = inode->i_ctime = E1;
+ E2 = E1;
)
::
cocci/ctime2.cocci
::
// get the places that set individual timespec64 fields
@@
struct inode *inode;
expression val, val2;
@@
- inode->i_ctime.tv_sec = val
+ inode_set_ctime(inode, val, val2)
...
- inode->i_ctime.tv_nsec = val2;
// get places that just set the tv_sec
@@
struct inode *inode;
expression sec, E1, E2, E3;
@@
(
- E3 = inode->i_ctime.tv_sec = sec
+ E3 = inode_set_ctime(inode, sec, 0).tv_sec
|
- inode->i_ctime.tv_sec = sec
+ inode_set_ctime(inode, sec, 0)
)
<...
(
- inode->i_ctime.tv_nsec = 0;
|
- E1 = inode->i_ctime.tv_nsec = 0
+ E1 = 0
|
- inode->i_ctime.tv_nsec = E1 = 0
+ E1 = 0
|
- inode->i_ctime.tv_nsec = E1 = E2 = 0
+ E1 = E2 = 0
)
...>
::
cocci/ctime3.cocci
::
// convert places that set i_ctime to a timespec64 directly
@@
struct inode *inode;
expression ts, E1, E2;
@@
(
- inode->i_ctime = E1 = E2 = ts
+ E1 = E2 = inode_set_ctime_to_ts(inode, ts)
|
- inode->i_ctime = E1 = ts
+ E1 = inode_set_ctime_to_ts(inode, ts)
|
- inode->i_ctime = ts
+ inode_set_ctime_to_ts(inode, ts)
)
::
cocci/ctime4.cocci
::
// catch places that set the i_ctime in an inode embedded in another structure
@@
expression E1, E2, E3;
@@
(
- E3.i_ctime = E1 = E2 = current_time()
+ E1 = E2 = inode_set_ctime_current()
|
- E3.i_ctime = E1 = current_time()
+ E1 = inode_set_ctime_current()
|
- E1 = E3.i_ctime = current_time()
+ E1 = inode_set_ctime_current()
|
- E3.i_ctime = current_time()
+ inode_set_ctime_current()
)
::
cocci/ctime5.cocci
::
// convert the remaining i_ctime accesses
@@
struct inode *inode;
@@
- inode->i_ctime
+ inode_get_ctime(inode)
Jeff Layton (92):
ibmvmc: update ctime in conjunction with mtime on write
bfs: update ctime in addition to mtime when adding entries
efivarfs: update ctime when mtime changes on a write
exfat: ensure that ctime is updated whenever the mtime is
apparmor: update ctime whenever the mtime changes on an inode
cifs: update the ctime on a partial page write
fs: add ctime accessors infrastructure
fs: new helper: simple_rename_timestamp
btrfs: convert to simple_rename_timestamp
ubifs: convert to simple_rename_timestamp
shmem: convert to simple_rename_timestamp
exfat: convert to simple_rename_timestamp
ntfs3: convert to simple_rename_timestamp
reiserfs: convert to simple_rename_timestamp
spufs: convert to ctime accessor functions
s390: convert to ctime accessor functions
binderfs: convert to ctime accessor functions
infiniband: convert to ctime accessor functions
ibm: convert to ctime accessor functions
usb: convert to ctime accessor functions
9p: convert to ctime accessor functions
adfs: convert to ctime accessor functions
affs: convert to ctime accessor functions
afs: convert to ctime accessor functions
fs: convert to ctime accessor functions
autofs: convert to ctime accessor functions
befs: convert to ctime
