* [PATCH v2 0/2] Update device mod time fixes @ 2021-10-14 17:10 Josef Bacik 2021-10-14 17:11 ` [PATCH v2 1/2] fs: export an inode_update_time helper Josef Bacik 2021-10-14 17:11 ` [PATCH v2 2/2] btrfs: update device path inode time instead of bd_inode Josef Bacik 0 siblings, 2 replies; 6+ messages in thread From: Josef Bacik @ 2021-10-14 17:10 UTC (permalink / raw) To: hch, linux-btrfs, kernel-team 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] fs: export an inode_update_time helper 2021-10-14 17:10 [PATCH v2 0/2] Update device mod time fixes Josef Bacik @ 2021-10-14 17:11 ` Josef Bacik 2021-10-21 16:38 ` David Sterba 2021-10-14 17:11 ` [PATCH v2 2/2] btrfs: update device path inode time instead of bd_inode Josef Bacik 1 sibling, 1 reply; 6+ messages in thread From: Josef Bacik @ 2021-10-14 17:11 UTC (permalink / raw) To: hch, linux-btrfs, kernel-team 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] fs: export an inode_update_time helper 2021-10-14 17:11 ` [PATCH v2 1/2] fs: export an inode_update_time helper Josef Bacik @ 2021-10-21 16:38 ` David Sterba 2021-10-25 7:45 ` Christoph Hellwig 0 siblings, 1 reply; 6+ messages in thread From: David Sterba @ 2021-10-21 16:38 UTC (permalink / raw) To: Josef Bacik; +Cc: hch, linux-btrfs, kernel-team 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] fs: export an inode_update_time helper 2021-10-21 16:38 ` David Sterba @ 2021-10-25 7:45 ` Christoph Hellwig 2021-10-25 17:36 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2021-10-25 7:45 UTC (permalink / raw) To: dsterba, Josef Bacik, hch, linux-btrfs, kernel-team 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] fs: export an inode_update_time helper 2021-10-25 7:45 ` Christoph Hellwig @ 2021-10-25 17:36 ` David Sterba 0 siblings, 0 replies; 6+ messages in thread From: David Sterba @ 2021-10-25 17:36 UTC (permalink / raw) To: Christoph Hellwig; +Cc: dsterba, Josef Bacik, linux-btrfs, kernel-team, Al Viro 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] btrfs: update device path inode time instead of bd_inode 2021-10-14 17:10 [PATCH v2 0/2] Update device mod time fixes Josef Bacik 2021-10-14 17:11 ` [PATCH v2 1/2] fs: export an inode_update_time helper Josef Bacik @ 2021-10-14 17:11 ` Josef Bacik 1 sibling, 0 replies; 6+ messages in thread From: Josef Bacik @ 2021-10-14 17:11 UTC (permalink / raw) To: hch, linux-btrfs, kernel-team 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-25 17:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-14 17:10 [PATCH v2 0/2] Update device mod time fixes Josef Bacik 2021-10-14 17:11 ` [PATCH v2 1/2] fs: export an inode_update_time helper Josef Bacik 2021-10-21 16:38 ` David Sterba 2021-10-25 7:45 ` Christoph Hellwig 2021-10-25 17:36 ` David Sterba 2021-10-14 17:11 ` [PATCH v2 2/2] btrfs: update device path inode time instead of bd_inode Josef Bacik
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.