[PATCH v2 0/2] Update device mod time fixes

[PATCH v2 0/2] Update device mod time fixes

[Josef Bacik]

Hello,

Christoph noticed that my original fix to stop calling filp_open() when we
remove devices was wrong because ->bd_inode isn't the inode we need to be
update, it needs to be the inode that is associated with the path to the device
node.

In order to do this I need to do a kern_path() to lookup the path to that inode,
and then I need to update the time for that inode.  generic_update_time() isn't
appropriate here as it should be used by ->update_time() if at all.  Instead
export a inode_update_time() helper to do the correct thing, and use that.

This allows us to update the time properly for libblkid, and makes sure we still
have the lockdep fix that was the motivation of the original fix.  Thanks,


Josef Bacik (2):
  fs: export an inode_update_time helper
  btrfs: update device path inode time instead of bd_inode

 fs/btrfs/volumes.c | 21 +++++++++++++--------
 fs/inode.c         |  7 ++++---
 include/linux/fs.h |  2 ++
 3 files changed, 19 insertions(+), 11 deletions(-)

-- 
2.26.3

[PATCH v2 1/2] fs: export an inode_update_time helper

[Josef Bacik]

If you already have an inode and need to update the time on the inode
there is no way to do this properly.  Export this helper to allow file
systems to update time on the inode so the appropriate handler is
called, either ->update_time or generic_update_time.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/inode.c         | 7 ++++---
 include/linux/fs.h | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index ed0cab8a32db..9abc88d7959c 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1782,12 +1782,13 @@ EXPORT_SYMBOL(generic_update_time);
  * This does the actual work of updating an inodes time or version.  Must have
  * had called mnt_want_write() before calling this.
  */
-static int update_time(struct inode *inode, struct timespec64 *time, int flags)
+int inode_update_time(struct inode *inode, struct timespec64 *time, int flags)
 {
 	if (inode->i_op->update_time)
 		return inode->i_op->update_time(inode, time, flags);
 	return generic_update_time(inode, time, flags);
 }
+EXPORT_SYMBOL(inode_update_time);
 
 /**
  *	atime_needs_update	-	update the access time
@@ -1857,7 +1858,7 @@ void touch_atime(const struct path *path)
 	 * of the fs read only, e.g. subvolumes in Btrfs.
 	 */
 	now = current_time(inode);
-	update_time(inode, &now, S_ATIME);
+	inode_update_time(inode, &now, S_ATIME);
 	__mnt_drop_write(mnt);
 skip_update:
 	sb_end_write(inode->i_sb);
@@ -2002,7 +2003,7 @@ int file_update_time(struct file *file)
 	if (__mnt_want_write_file(file))
 		return 0;
 
-	ret = update_time(inode, &now, sync_it);
+	ret = inode_update_time(inode, &now, sync_it);
 	__mnt_drop_write_file(file);
 
 	return ret;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e7a633353fd2..20e2fe398ab6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2498,6 +2498,8 @@ enum file_time_flags {
 
 extern bool atime_needs_update(const struct path *, struct inode *);
 extern void touch_atime(const struct path *);
+extern int inode_update_time(struct inode *inode, struct timespec64 *time,
+			     int flags);
 static inline void file_accessed(struct file *file)
 {
 	if (!(file->f_flags & O_NOATIME))
-- 
2.26.3

[PATCH v2 2/2] btrfs: update device path inode time instead of bd_inode

[Josef Bacik]

Christoph pointed out that I'm updating bdev->bd_inode for the device
time when we remove block devices from a btrfs file system, however this
isn't actually exposed to anything.  The inode we want to update is the
one that's associated with the path to the device, usually on devtmpfs,
so that blkid notices the difference.

We still don't want to do the blkdev_open, so use kern_path() to get the
path to the given device and do the update time on that inode.

Fixes: 8f96a5bfa150 ("btrfs: update the bdev time directly when closing")
Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/volumes.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 3262e75fbb1c..34729f0e4a9b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -14,6 +14,7 @@
 #include <linux/semaphore.h>
 #include <linux/uuid.h>
 #include <linux/list_sort.h>
+#include <linux/namei.h>
 #include "misc.h"
 #include "ctree.h"
 #include "extent_map.h"
@@ -1886,18 +1887,22 @@ static int btrfs_add_dev_item(struct btrfs_trans_handle *trans,
 /*
  * Function to update ctime/mtime for a given device path.
  * Mainly used for ctime/mtime based probe like libblkid.
+ *
+ * We don't care about errors here, this is just to be kind to userspace.
  */
-static void update_dev_time(struct block_device *bdev)
+static void update_dev_time(const char *device_path)
 {
-	struct inode *inode = bdev->bd_inode;
+	struct path path;
 	struct timespec64 now;
+	int ret;
 
-	/* Shouldn't happen but just in case. */
-	if (!inode)
+	ret = kern_path(device_path, LOOKUP_FOLLOW, &path);
+	if (ret)
 		return;
 
-	now = current_time(inode);
-	generic_update_time(inode, &now, S_MTIME | S_CTIME);
+	now = current_time(d_inode(path.dentry));
+	inode_update_time(d_inode(path.dentry), &now, S_MTIME | S_CTIME);
+	path_put(&path);
 }
 
 static int btrfs_rm_dev_item(struct btrfs_device *device)
@@ -2073,7 +2078,7 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
 	btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
 
 	/* Update ctime/mtime for device path for libblkid */
-	update_dev_time(bdev);
+	update_dev_time(device_path);
 }
 
 int btrfs_rm_device(struct btrfs_fs_info *fs_info,
@@ -2767,7 +2772,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	btrfs_forget_devices(device_path);
 
 	/* Update ctime/mtime for blkid or udev */
-	update_dev_time(bdev);
+	update_dev_time(device_path);
 
 	return ret;
 
-- 
2.26.3

Re: [PATCH v2 1/2] fs: export an inode_update_time helper

[David Sterba]

On Thu, Oct 14, 2021 at 01:11:00PM -0400, Josef Bacik wrote:
> If you already have an inode and need to update the time on the inode
> there is no way to do this properly.  Export this helper to allow file
> systems to update time on the inode so the appropriate handler is
> called, either ->update_time or generic_update_time.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

I'd like to get ack from Christoph, though it's a simple change it's
still in another subsystem.

> ---
>  fs/inode.c         | 7 ++++---
>  include/linux/fs.h | 2 ++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index ed0cab8a32db..9abc88d7959c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1782,12 +1782,13 @@ EXPORT_SYMBOL(generic_update_time);
>   * This does the actual work of updating an inodes time or version.  Must have
>   * had called mnt_want_write() before calling this.
>   */
> -static int update_time(struct inode *inode, struct timespec64 *time, int flags)
> +int inode_update_time(struct inode *inode, struct timespec64 *time, int flags)
>  {
>  	if (inode->i_op->update_time)
>  		return inode->i_op->update_time(inode, time, flags);
>  	return generic_update_time(inode, time, flags);
>  }
> +EXPORT_SYMBOL(inode_update_time);
>  
>  /**
>   *	atime_needs_update	-	update the access time
> @@ -1857,7 +1858,7 @@ void touch_atime(const struct path *path)
>  	 * of the fs read only, e.g. subvolumes in Btrfs.
>  	 */
>  	now = current_time(inode);
> -	update_time(inode, &now, S_ATIME);
> +	inode_update_time(inode, &now, S_ATIME);
>  	__mnt_drop_write(mnt);
>  skip_update:
>  	sb_end_write(inode->i_sb);
> @@ -2002,7 +2003,7 @@ int file_update_time(struct file *file)
>  	if (__mnt_want_write_file(file))
>  		return 0;
>  
> -	ret = update_time(inode, &now, sync_it);
> +	ret = inode_update_time(inode, &now, sync_it);
>  	__mnt_drop_write_file(file);
>  
>  	return ret;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e7a633353fd2..20e2fe398ab6 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2498,6 +2498,8 @@ enum file_time_flags {
>  
>  extern bool atime_needs_update(const struct path *, struct inode *);
>  extern void touch_atime(const struct path *);
> +extern int inode_update_time(struct inode *inode, struct timespec64 *time,
> +			     int flags);

As was pointed out in the past for similar patches, the 'extern' can be
dropped, so I'll that.

Re: [PATCH v2 1/2] fs: export an inode_update_time helper

[Christoph Hellwig]

On Thu, Oct 21, 2021 at 06:38:17PM +0200, David Sterba wrote:
> On Thu, Oct 14, 2021 at 01:11:00PM -0400, Josef Bacik wrote:
> > If you already have an inode and need to update the time on the inode
> > there is no way to do this properly.  Export this helper to allow file
> > systems to update time on the inode so the appropriate handler is
> > called, either ->update_time or generic_update_time.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> I'd like to get ack from Christoph, though it's a simple change it's
> still in another subsystem.

Not a big fan, but compared to the other options it seems like the
least bad option.  That being said I'm not the VFS maintainer anyway.

Re: [PATCH v2 1/2] fs: export an inode_update_time helper

[David Sterba]

On Mon, Oct 25, 2021 at 09:45:09AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 21, 2021 at 06:38:17PM +0200, David Sterba wrote:
> > On Thu, Oct 14, 2021 at 01:11:00PM -0400, Josef Bacik wrote:
> > > If you already have an inode and need to update the time on the inode
> > > there is no way to do this properly.  Export this helper to allow file
> > > systems to update time on the inode so the appropriate handler is
> > > called, either ->update_time or generic_update_time.
> > > 
> > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > 
> > I'd like to get ack from Christoph, though it's a simple change it's
> > still in another subsystem.
> 
> Not a big fan, but compared to the other options it seems like the
> least bad option.

Thanks, given that it fixes the time update problem and is otherwise
quite unintrusive it should be ok.

> That being said I'm not the VFS maintainer anyway.

Well yeah, but you reviewed or was involved in some other similar
changes, so I take it as a confirmation that we're not abusing some VFS
internal.  But I'm adding Al Viro to CC anyway.