| Message ID | 20220930111840.10695-9-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs/nfsd: clean up handling of i_version counter | expand |
Hi Jeff,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20220929]
[cannot apply to tytso-ext4/dev trondmy-nfs/linux-next ceph-client/for-linus linus/master v6.0-rc7 v6.0-rc6 v6.0-rc5 v6.0-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jeff-Layton/vfs-nfsd-clean-up-handling-of-i_version-counter/20220930-192844
base: 1c6c4f42b3de4f18ea96d62950d0e266ca35a055
config: i386-randconfig-a001
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/b305ff6981591917824b59e5ac7f833afea77ce6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jeff-Layton/vfs-nfsd-clean-up-handling-of-i_version-counter/20220930-192844
git checkout b305ff6981591917824b59e5ac7f833afea77ce6
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/bug.h:87,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from arch/x86/include/asm/preempt.h:7,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:56,
from include/linux/wait.h:9,
from include/linux/wait_bit.h:8,
from include/linux/fs.h:6,
from include/linux/dax.h:5,
from mm/filemap.c:15:
mm/filemap.c: In function '__generic_file_write_iter':
>> mm/filemap.c:3892:32: warning: format '%ld' expects argument of type 'long int', but argument 2 has type 'ssize_t' {aka 'int'} [-Wformat=]
3892 | WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ~~~
| |
| ssize_t {aka int}
include/asm-generic/bug.h:105:31: note: in definition of macro '__WARN_printf'
105 | __warn_printk(arg); \
| ^~~
include/linux/once_lite.h:31:25: note: in expansion of macro 'WARN'
31 | func(__VA_ARGS__); \
| ^~~~
include/asm-generic/bug.h:151:9: note: in expansion of macro 'DO_ONCE_LITE_IF'
151 | DO_ONCE_LITE_IF(condition, WARN, 1, format)
| ^~~~~~~~~~~~~~~
mm/filemap.c:3892:17: note: in expansion of macro 'WARN_ONCE'
3892 | WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err);
| ^~~~~~~~~
mm/filemap.c:3892:73: note: format string is defined here
3892 | WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err);
| ~~^
| |
| long int
| %d
vim +3892 mm/filemap.c
3793
3794 /**
3795 * __generic_file_write_iter - write data to a file
3796 * @iocb: IO state structure (file, offset, etc.)
3797 * @from: iov_iter with data to write
3798 *
3799 * This function does all the work needed for actually writing data to a
3800 * file. It does all basic checks, removes SUID from the file, updates
3801 * modification times and calls proper subroutines depending on whether we
3802 * do direct IO or a standard buffered write.
3803 *
3804 * It expects i_rwsem to be grabbed unless we work on a block device or similar
3805 * object which does not need locking at all.
3806 *
3807 * This function does *not* take care of syncing data in case of O_SYNC write.
3808 * A caller has to handle it. This is mainly due to the fact that we want to
3809 * avoid syncing under i_rwsem.
3810 *
3811 * Return:
3812 * * number of bytes written, even for truncated writes
3813 * * negative error code if no data has been written at all
3814 */
3815 ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
3816 {
3817 struct file *file = iocb->ki_filp;
3818 struct address_space *mapping = file->f_mapping;
3819 struct inode *inode = mapping->host;
3820 ssize_t written = 0;
3821 ssize_t err;
3822 ssize_t status;
3823
3824 /* We can write back this queue in page reclaim */
3825 current->backing_dev_info = inode_to_bdi(inode);
3826 err = file_remove_privs(file);
3827 if (err)
3828 goto out;
3829
3830 if (iocb->ki_flags & IOCB_DIRECT) {
3831 loff_t pos, endbyte;
3832
3833 written = generic_file_direct_write(iocb, from);
3834 /*
3835 * If the write stopped short of completing, fall back to
3836 * buffered writes. Some filesystems do this for writes to
3837 * holes, for example. For DAX files, a buffered write will
3838 * not succeed (even if it did, DAX does not handle dirty
3839 * page-cache pages correctly).
3840 */
3841 if (written < 0 || !iov_iter_count(from) || IS_DAX(inode))
3842 goto out;
3843
3844 pos = iocb->ki_pos;
3845 status = generic_perform_write(iocb, from);
3846 /*
3847 * If generic_perform_write() returned a synchronous error
3848 * then we want to return the number of bytes which were
3849 * direct-written, or the error code if that was zero. Note
3850 * that this differs from normal direct-io semantics, which
3851 * will return -EFOO even if some bytes were written.
3852 */
3853 if (unlikely(status < 0)) {
3854 err = status;
3855 goto out;
3856 }
3857 /*
3858 * We need to ensure that the page cache pages are written to
3859 * disk and invalidated to preserve the expected O_DIRECT
3860 * semantics.
3861 */
3862 endbyte = pos + status - 1;
3863 err = filemap_write_and_wait_range(mapping, pos, endbyte);
3864 if (err == 0) {
3865 iocb->ki_pos = endbyte + 1;
3866 written += status;
3867 invalidate_mapping_pages(mapping,
3868 pos >> PAGE_SHIFT,
3869 endbyte >> PAGE_SHIFT);
3870 } else {
3871 /*
3872 * We don't know how much we wrote, so just return
3873 * the number of bytes which were direct-written
3874 */
3875 }
3876 } else {
3877 written = generic_perform_write(iocb, from);
3878 if (likely(written > 0))
3879 iocb->ki_pos += written;
3880 }
3881 out:
3882 if (written > 0) {
3883 err = file_update_time(file);
3884 /*
3885 * There isn't much we can do at this point if updating the
3886 * times fails after a successful write. The times and i_version
3887 * should still be updated in the inode, and it should still be
3888 * marked dirty, so hopefully the next inode update will catch it.
3889 * Log a warning once so we have a record that something untoward
3890 * has occurred.
3891 */
> 3892 WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err);
3893 }
3894
3895 current->backing_dev_info = NULL;
3896 return written ? written : err;
3897 }
3898 EXPORT_SYMBOL(__generic_file_write_iter);
3899
Hi Jeff,
I love your patch! Perhaps something to improve:
[auto build test WARNING on next-20220929]
[cannot apply to tytso-ext4/dev trondmy-nfs/linux-next ceph-client/for-linus linus/master v6.0-rc7 v6.0-rc6 v6.0-rc5 v6.0-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jeff-Layton/vfs-nfsd-clean-up-handling-of-i_version-counter/20220930-192844
base: 1c6c4f42b3de4f18ea96d62950d0e266ca35a055
config: i386-randconfig-a002
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/b305ff6981591917824b59e5ac7f833afea77ce6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jeff-Layton/vfs-nfsd-clean-up-handling-of-i_version-counter/20220930-192844
git checkout b305ff6981591917824b59e5ac7f833afea77ce6
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> mm/filemap.c:3892:65: warning: format specifies type 'long' but the argument has type 'ssize_t' (aka 'int') [-Wformat]
WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err);
~~~ ^~~
%zd
include/asm-generic/bug.h:151:38: note: expanded from macro 'WARN_ONCE'
DO_ONCE_LITE_IF(condition, WARN, 1, format)
^~~~~~
include/linux/once_lite.h:31:9: note: expanded from macro 'DO_ONCE_LITE_IF'
func(__VA_ARGS__); \
^~~~~~~~~~~
include/asm-generic/bug.h:133:29: note: expanded from macro 'WARN'
__WARN_printf(TAINT_WARN, format); \
^~~~~~
include/asm-generic/bug.h:105:17: note: expanded from macro '__WARN_printf'
__warn_printk(arg); \
^~~
1 warning generated.
vim +3892 mm/filemap.c
3793
3794 /**
3795 * __generic_file_write_iter - write data to a file
3796 * @iocb: IO state structure (file, offset, etc.)
3797 * @from: iov_iter with data to write
3798 *
3799 * This function does all the work needed for actually writing data to a
3800 * file. It does all basic checks, removes SUID from the file, updates
3801 * modification times and calls proper subroutines depending on whether we
3802 * do direct IO or a standard buffered write.
3803 *
3804 * It expects i_rwsem to be grabbed unless we work on a block device or similar
3805 * object which does not need locking at all.
3806 *
3807 * This function does *not* take care of syncing data in case of O_SYNC write.
3808 * A caller has to handle it. This is mainly due to the fact that we want to
3809 * avoid syncing under i_rwsem.
3810 *
3811 * Return:
3812 * * number of bytes written, even for truncated writes
3813 * * negative error code if no data has been written at all
3814 */
3815 ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
3816 {
3817 struct file *file = iocb->ki_filp;
3818 struct address_space *mapping = file->f_mapping;
3819 struct inode *inode = mapping->host;
3820 ssize_t written = 0;
3821 ssize_t err;
3822 ssize_t status;
3823
3824 /* We can write back this queue in page reclaim */
3825 current->backing_dev_info = inode_to_bdi(inode);
3826 err = file_remove_privs(file);
3827 if (err)
3828 goto out;
3829
3830 if (iocb->ki_flags & IOCB_DIRECT) {
3831 loff_t pos, endbyte;
3832
3833 written = generic_file_direct_write(iocb, from);
3834 /*
3835 * If the write stopped short of completing, fall back to
3836 * buffered writes. Some filesystems do this for writes to
3837 * holes, for example. For DAX files, a buffered write will
3838 * not succeed (even if it did, DAX does not handle dirty
3839 * page-cache pages correctly).
3840 */
3841 if (written < 0 || !iov_iter_count(from) || IS_DAX(inode))
3842 goto out;
3843
3844 pos = iocb->ki_pos;
3845 status = generic_perform_write(iocb, from);
3846 /*
3847 * If generic_perform_write() returned a synchronous error
3848 * then we want to return the number of bytes which were
3849 * direct-written, or the error code if that was zero. Note
3850 * that this differs from normal direct-io semantics, which
3851 * will return -EFOO even if some bytes were written.
3852 */
3853 if (unlikely(status < 0)) {
3854 err = status;
3855 goto out;
3856 }
3857 /*
3858 * We need to ensure that the page cache pages are written to
3859 * disk and invalidated to preserve the expected O_DIRECT
3860 * semantics.
3861 */
3862 endbyte = pos + status - 1;
3863 err = filemap_write_and_wait_range(mapping, pos, endbyte);
3864 if (err == 0) {
3865 iocb->ki_pos = endbyte + 1;
3866 written += status;
3867 invalidate_mapping_pages(mapping,
3868 pos >> PAGE_SHIFT,
3869 endbyte >> PAGE_SHIFT);
3870 } else {
3871 /*
3872 * We don't know how much we wrote, so just return
3873 * the number of bytes which were direct-written
3874 */
3875 }
3876 } else {
3877 written = generic_perform_write(iocb, from);
3878 if (likely(written > 0))
3879 iocb->ki_pos += written;
3880 }
3881 out:
3882 if (written > 0) {
3883 err = file_update_time(file);
3884 /*
3885 * There isn't much we can do at this point if updating the
3886 * times fails after a successful write. The times and i_version
3887 * should still be updated in the inode, and it should still be
3888 * marked dirty, so hopefully the next inode update will catch it.
3889 * Log a warning once so we have a record that something untoward
3890 * has occurred.
3891 */
> 3892 WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err);
3893 }
3894
3895 current->backing_dev_info = NULL;
3896 return written ? written : err;
3897 }
3898 EXPORT_SYMBOL(__generic_file_write_iter);
3899
On Fri, Sep 30, 2022 at 2:30 PM Jeff Layton <jlayton@kernel.org> wrote: > > The c/mtime and i_version currently get updated before the data is > copied (or a DIO write is issued), which is problematic for NFS. > > READ+GETATTR can race with a write (even a local one) in such a way as > to make the client associate the state of the file with the wrong change > attribute. That association can persist indefinitely if the file sees no > further changes. > > Move the setting of times to the bottom of the function in > __generic_file_write_iter and only update it if something was > successfully written. > This solution is wrong for several reasons: 1. There is still file_update_time() in ->page_mkwrite() so you haven't solved the problem completely 2. The other side of the coin is that post crash state is more likely to end up data changes without mtime/ctime change If I read the problem description correctly, then a solution that invalidates the NFS cache before AND after the write would be acceptable. Right? Would an extra i_version bump after the write solve the race? > If the time update fails, log a warning once, but don't fail the write. > All of the existing callers use update_time functions that don't fail, > so we should never trip this. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > mm/filemap.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/mm/filemap.c b/mm/filemap.c > index 15800334147b..72c0ceb75176 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -3812,10 +3812,6 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > if (err) > goto out; > > - err = file_update_time(file); > - if (err) > - goto out; > - > if (iocb->ki_flags & IOCB_DIRECT) { > loff_t pos, endbyte; > > @@ -3868,6 +3864,19 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > iocb->ki_pos += written; > } > out: > + if (written > 0) { > + err = file_update_time(file); > + /* > + * There isn't much we can do at this point if updating the > + * times fails after a successful write. The times and i_version > + * should still be updated in the inode, and it should still be > + * marked dirty, so hopefully the next inode update will catch it. > + * Log a warning once so we have a record that something untoward > + * has occurred. > + */ > + WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err); pr_warn_once() please - this is not a programming assertion. Thanks, Amir.
On Sun, 2022-10-02 at 10:08 +0300, Amir Goldstein wrote: > On Fri, Sep 30, 2022 at 2:30 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > The c/mtime and i_version currently get updated before the data is > > copied (or a DIO write is issued), which is problematic for NFS. > > > > READ+GETATTR can race with a write (even a local one) in such a way as > > to make the client associate the state of the file with the wrong change > > attribute. That association can persist indefinitely if the file sees no > > further changes. > > > > Move the setting of times to the bottom of the function in > > __generic_file_write_iter and only update it if something was > > successfully written. > > > > This solution is wrong for several reasons: > > 1. There is still file_update_time() in ->page_mkwrite() so you haven't > solved the problem completely Right. I don't think there is a way to solve the problem vs. mmap. Userland can write to a writeable mmap'ed page at any time and we'd never know. We have to specifically carve out mmap as an exception here. I'll plan to add something to the manpage patch for this. > 2. The other side of the coin is that post crash state is more likely to end > up data changes without mtime/ctime change > Is this really something filesystems rely on? I suppose the danger is that some cached data gets written to disk before the write returns and the inode on disk never gets updated. But...isn't that a danger now? Some of the cached data could get written out and the updated inode just never makes it to disk before a crash (AFAIU). I'm not sure that this increases our exposure to that problem. > If I read the problem description correctly, then a solution that invalidates > the NFS cache before AND after the write would be acceptable. Right? > Would an extra i_version bump after the write solve the race? > I based this patch on Neil's assertion that updating the time before an operation was pointless if we were going to do it afterward. The NFS client only really cares about seeing it change after a write. Doing both would be fine from a correctness standpoint, and in most cases, the second would be a no-op anyway since a query would have to race in between the two for that to happen. FWIW, I think we should update the m/ctime and version at the same time. If the version changes, then there is always the potential that a timer tick has occurred. So, that would translate to a second call to file_update_time in here. The downside of bumping the times/version both before and after is that these are hot codepaths, and we'd be adding extra operations there. Even in the case where nothing has changed, we'd have to call inode_needs_update_time a second time for every write. Is that worth the cost? > > If the time update fails, log a warning once, but don't fail the write. > > All of the existing callers use update_time functions that don't fail, > > so we should never trip this. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > mm/filemap.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > index 15800334147b..72c0ceb75176 100644 > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -3812,10 +3812,6 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > > if (err) > > goto out; > > > > - err = file_update_time(file); > > - if (err) > > - goto out; > > - > > if (iocb->ki_flags & IOCB_DIRECT) { > > loff_t pos, endbyte; > > > > @@ -3868,6 +3864,19 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from) > > iocb->ki_pos += written; > > } > > out: > > + if (written > 0) { > > + err = file_update_time(file); > > + /* > > + * There isn't much we can do at this point if updating the > > + * times fails after a successful write. The times and i_version > > + * should still be updated in the inode, and it should still be > > + * marked dirty, so hopefully the next inode update will catch it. > > + * Log a warning once so we have a record that something untoward > > + * has occurred. > > + */ > > + WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err); > > pr_warn_once() please - this is not a programming assertion. > ACK. I'll change that.
On Mon, Oct 3, 2022 at 4:01 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Sun, 2022-10-02 at 10:08 +0300, Amir Goldstein wrote: > > On Fri, Sep 30, 2022 at 2:30 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > The c/mtime and i_version currently get updated before the data is > > > copied (or a DIO write is issued), which is problematic for NFS. > > > > > > READ+GETATTR can race with a write (even a local one) in such a way as > > > to make the client associate the state of the file with the wrong change > > > attribute. That association can persist indefinitely if the file sees no > > > further changes. > > > > > > Move the setting of times to the bottom of the function in > > > __generic_file_write_iter and only update it if something was > > > successfully written. > > > > > > > This solution is wrong for several reasons: > > > > 1. There is still file_update_time() in ->page_mkwrite() so you haven't > > solved the problem completely > > Right. I don't think there is a way to solve the problem vs. mmap. > Userland can write to a writeable mmap'ed page at any time and we'd > never know. We have to specifically carve out mmap as an exception here. > I'll plan to add something to the manpage patch for this. > > > 2. The other side of the coin is that post crash state is more likely to end > > up data changes without mtime/ctime change > > > > Is this really something filesystems rely on? I suppose the danger is > that some cached data gets written to disk before the write returns and > the inode on disk never gets updated. > > But...isn't that a danger now? Some of the cached data could get written > out and the updated inode just never makes it to disk before a crash > (AFAIU). I'm not sure that this increases our exposure to that problem. > > You are correct that that danger exists, but it only exists for overwriting to allocated blocks. For writing to new blocks, mtime change is recorded in transaction before the block mapping is recorded in transaction so there is no danger in this case (before your patch). Also, observing size change without observing mtime change after crash seems like a very bad outcome that may be possible after your change. These are just a few cases that I could think of, they may be filesystem dependent, but my gut feeling is that if you remove the time update before the operation, that has been like that forever, a lot of s#!t is going to float for various filesystems and applications. And it is not one of those things that are discovered during rc or even stable kernel testing - they are discovered much later when users start to realize their applications got bogged up after crash, so it feels like to me like playing with fire. > > If I read the problem description correctly, then a solution that invalidates > > the NFS cache before AND after the write would be acceptable. Right? > > Would an extra i_version bump after the write solve the race? > > > > I based this patch on Neil's assertion that updating the time before an > operation was pointless if we were going to do it afterward. The NFS > client only really cares about seeing it change after a write. > Pointless to NFS client maybe. Whether or not this is not changing user behavior for other applications is up to you to prove and I doubt that you can prove it because I doubt that it is true. > Doing both would be fine from a correctness standpoint, and in most > cases, the second would be a no-op anyway since a query would have to > race in between the two for that to happen. > > FWIW, I think we should update the m/ctime and version at the same time. > If the version changes, then there is always the potential that a timer > tick has occurred. So, that would translate to a second call to > file_update_time in here. > > The downside of bumping the times/version both before and after is that > these are hot codepaths, and we'd be adding extra operations there. Even > in the case where nothing has changed, we'd have to call > inode_needs_update_time a second time for every write. Is that worth the > cost? Is there a practical cost for iversion bump AFTER write as I suggested? If you NEED m/ctime update AFTER write and iversion update is not enough then I did not understand from your commit message why that is. Thanks, Amir.
On Tue, 04 Oct 2022, Amir Goldstein wrote: > On Mon, Oct 3, 2022 at 4:01 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Sun, 2022-10-02 at 10:08 +0300, Amir Goldstein wrote: > > > On Fri, Sep 30, 2022 at 2:30 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > The c/mtime and i_version currently get updated before the data is > > > > copied (or a DIO write is issued), which is problematic for NFS. > > > > > > > > READ+GETATTR can race with a write (even a local one) in such a way as > > > > to make the client associate the state of the file with the wrong change > > > > attribute. That association can persist indefinitely if the file sees no > > > > further changes. > > > > > > > > Move the setting of times to the bottom of the function in > > > > __generic_file_write_iter and only update it if something was > > > > successfully written. > > > > > > > > > > This solution is wrong for several reasons: > > > > > > 1. There is still file_update_time() in ->page_mkwrite() so you haven't > > > solved the problem completely > > > > Right. I don't think there is a way to solve the problem vs. mmap. > > Userland can write to a writeable mmap'ed page at any time and we'd > > never know. We have to specifically carve out mmap as an exception here. > > I'll plan to add something to the manpage patch for this. > > > > > 2. The other side of the coin is that post crash state is more likely to end > > > up data changes without mtime/ctime change > > > > > > > Is this really something filesystems rely on? I suppose the danger is > > that some cached data gets written to disk before the write returns and > > the inode on disk never gets updated. > > > > But...isn't that a danger now? Some of the cached data could get written > > out and the updated inode just never makes it to disk before a crash > > (AFAIU). I'm not sure that this increases our exposure to that problem. > > > > > > You are correct that that danger exists, but it only exists for overwriting > to allocated blocks. > > For writing to new blocks, mtime change is recorded in transaction > before the block mapping is recorded in transaction so there is no > danger in this case (before your patch). > > Also, observing size change without observing mtime change > after crash seems like a very bad outcome that may be possible > after your change. > > These are just a few cases that I could think of, they may be filesystem > dependent, but my gut feeling is that if you remove the time update before > the operation, that has been like that forever, a lot of s#!t is going to float > for various filesystems and applications. > > And it is not one of those things that are discovered during rc or even > stable kernel testing - they are discovered much later when users start to > realize their applications got bogged up after crash, so it feels like to me > like playing with fire. > > > > If I read the problem description correctly, then a solution that invalidates > > > the NFS cache before AND after the write would be acceptable. Right? > > > Would an extra i_version bump after the write solve the race? > > > > > > > I based this patch on Neil's assertion that updating the time before an > > operation was pointless if we were going to do it afterward. The NFS > > client only really cares about seeing it change after a write. > > > > Pointless to NFS client maybe. > Whether or not this is not changing user behavior for other applications > is up to you to prove and I doubt that you can prove it because I doubt > that it is true. > > > Doing both would be fine from a correctness standpoint, and in most > > cases, the second would be a no-op anyway since a query would have to > > race in between the two for that to happen. > > > > FWIW, I think we should update the m/ctime and version at the same time. > > If the version changes, then there is always the potential that a timer > > tick has occurred. So, that would translate to a second call to > > file_update_time in here. > > > > The downside of bumping the times/version both before and after is that > > these are hot codepaths, and we'd be adding extra operations there. Even > > in the case where nothing has changed, we'd have to call > > inode_needs_update_time a second time for every write. Is that worth the > > cost? > > Is there a practical cost for iversion bump AFTER write as I suggested? > If you NEED m/ctime update AFTER write and iversion update is not enough > then I did not understand from your commit message why that is. > > Thanks, > Amir. > Maybe we should split i_version updates from ctime updates. While it isn't true that ctime updates have happened before the write "forever" it has been true since 2.3.43[1] which is close to forever. For ctime there doesn't appear to be a strong specification of when the change happens, so history provides a good case for leaving it before. For i_version we want to provide clear and unambiguous semantics. Performing 2 updates makes the specification muddy. So I would prefer a single update for i_version, performed after the change becomes visible. If that means it has to be separate from ctime, then so be it. NeilBrown [1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=636b38438001a00b25f23e38747a91cb8428af29
On Tue, 2022-10-04 at 09:56 +1100, NeilBrown wrote: > On Tue, 04 Oct 2022, Amir Goldstein wrote: > > On Mon, Oct 3, 2022 at 4:01 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > On Sun, 2022-10-02 at 10:08 +0300, Amir Goldstein wrote: > > > > On Fri, Sep 30, 2022 at 2:30 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > > > The c/mtime and i_version currently get updated before the data is > > > > > copied (or a DIO write is issued), which is problematic for NFS. > > > > > > > > > > READ+GETATTR can race with a write (even a local one) in such a way as > > > > > to make the client associate the state of the file with the wrong change > > > > > attribute. That association can persist indefinitely if the file sees no > > > > > further changes. > > > > > > > > > > Move the setting of times to the bottom of the function in > > > > > __generic_file_write_iter and only update it if something was > > > > > successfully written. > > > > > > > > > > > > > This solution is wrong for several reasons: > > > > > > > > 1. There is still file_update_time() in ->page_mkwrite() so you haven't > > > > solved the problem completely > > > > > > Right. I don't think there is a way to solve the problem vs. mmap. > > > Userland can write to a writeable mmap'ed page at any time and we'd > > > never know. We have to specifically carve out mmap as an exception here. > > > I'll plan to add something to the manpage patch for this. > > > > > > > 2. The other side of the coin is that post crash state is more likely to end > > > > up data changes without mtime/ctime change > > > > > > > > > > Is this really something filesystems rely on? I suppose the danger is > > > that some cached data gets written to disk before the write returns and > > > the inode on disk never gets updated. > > > > > > But...isn't that a danger now? Some of the cached data could get written > > > out and the updated inode just never makes it to disk before a crash > > > (AFAIU). I'm not sure that this increases our exposure to that problem. > > > > > > > > > > You are correct that that danger exists, but it only exists for overwriting > > to allocated blocks. > > > > For writing to new blocks, mtime change is recorded in transaction > > before the block mapping is recorded in transaction so there is no > > danger in this case (before your patch). > > > > Also, observing size change without observing mtime change > > after crash seems like a very bad outcome that may be possible > > after your change. > > > > These are just a few cases that I could think of, they may be filesystem > > dependent, but my gut feeling is that if you remove the time update before > > the operation, that has been like that forever, a lot of s#!t is going to float > > for various filesystems and applications. > > > > And it is not one of those things that are discovered during rc or even > > stable kernel testing - they are discovered much later when users start to > > realize their applications got bogged up after crash, so it feels like to me > > like playing with fire. > > > > > > If I read the problem description correctly, then a solution that invalidates > > > > the NFS cache before AND after the write would be acceptable. Right? > > > > Would an extra i_version bump after the write solve the race? > > > > > > > > > > I based this patch on Neil's assertion that updating the time before an > > > operation was pointless if we were going to do it afterward. The NFS > > > client only really cares about seeing it change after a write. > > > > > > > Pointless to NFS client maybe. > > Whether or not this is not changing user behavior for other applications > > is up to you to prove and I doubt that you can prove it because I doubt > > that it is true. > > > > > Doing both would be fine from a correctness standpoint, and in most > > > cases, the second would be a no-op anyway since a query would have to > > > race in between the two for that to happen. > > > > > > FWIW, I think we should update the m/ctime and version at the same time. > > > If the version changes, then there is always the potential that a timer > > > tick has occurred. So, that would translate to a second call to > > > file_update_time in here. > > > > > > The downside of bumping the times/version both before and after is that > > > these are hot codepaths, and we'd be adding extra operations there. Even > > > in the case where nothing has changed, we'd have to call > > > inode_needs_update_time a second time for every write. Is that worth the > > > cost? > > > > Is there a practical cost for iversion bump AFTER write as I suggested? > > If you NEED m/ctime update AFTER write and iversion update is not enough > > then I did not understand from your commit message why that is. > > > > Thanks, > > Amir. > > > > Maybe we should split i_version updates from ctime updates. > > While it isn't true that ctime updates have happened before the write > "forever" it has been true since 2.3.43[1] which is close to forever. > > For ctime there doesn't appear to be a strong specification of when the > change happens, so history provides a good case for leaving it before. > For i_version we want to provide clear and unambiguous semantics. > Performing 2 updates makes the specification muddy. > > So I would prefer a single update for i_version, performed after the > change becomes visible. If that means it has to be separate from ctime, > then so be it. > > NeilBrown > > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=636b38438001a00b25f23e38747a91cb8428af29 Not necessarily. We can document it in such a way that bumping it twice is allowed, but not required. My main concern with splitting them up is that we'd have to dirty the inode twice if both the times and the i_version need updating. If the inode gets written out in between, then we end up doing twice the I/O. The interim on-disk metadata would be in sort of a weird state too -- the ctime would have changed but the version would still be old. It might be worthwhile to just go ahead and continue bumping it in file_update_time, and then we'd just attempt to bump the i_version again afterward. The second bump will almost always be a no-op anyway.
On Thu, 06 Oct 2022, Jeff Layton wrote: > On Tue, 2022-10-04 at 09:56 +1100, NeilBrown wrote: > > On Tue, 04 Oct 2022, Amir Goldstein wrote: > > > On Mon, Oct 3, 2022 at 4:01 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > On Sun, 2022-10-02 at 10:08 +0300, Amir Goldstein wrote: > > > > > On Fri, Sep 30, 2022 at 2:30 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > > > > > > > > > The c/mtime and i_version currently get updated before the data is > > > > > > copied (or a DIO write is issued), which is problematic for NFS. > > > > > > > > > > > > READ+GETATTR can race with a write (even a local one) in such a way as > > > > > > to make the client associate the state of the file with the wrong change > > > > > > attribute. That association can persist indefinitely if the file sees no > > > > > > further changes. > > > > > > > > > > > > Move the setting of times to the bottom of the function in > > > > > > __generic_file_write_iter and only update it if something was > > > > > > successfully written. > > > > > > > > > > > > > > > > This solution is wrong for several reasons: > > > > > > > > > > 1. There is still file_update_time() in ->page_mkwrite() so you haven't > > > > > solved the problem completely > > > > > > > > Right. I don't think there is a way to solve the problem vs. mmap. > > > > Userland can write to a writeable mmap'ed page at any time and we'd > > > > never know. We have to specifically carve out mmap as an exception here. > > > > I'll plan to add something to the manpage patch for this. > > > > > > > > > 2. The other side of the coin is that post crash state is more likely to end > > > > > up data changes without mtime/ctime change > > > > > > > > > > > > > Is this really something filesystems rely on? I suppose the danger is > > > > that some cached data gets written to disk before the write returns and > > > > the inode on disk never gets updated. > > > > > > > > But...isn't that a danger now? Some of the cached data could get written > > > > out and the updated inode just never makes it to disk before a crash > > > > (AFAIU). I'm not sure that this increases our exposure to that problem. > > > > > > > > > > > > > > You are correct that that danger exists, but it only exists for overwriting > > > to allocated blocks. > > > > > > For writing to new blocks, mtime change is recorded in transaction > > > before the block mapping is recorded in transaction so there is no > > > danger in this case (before your patch). > > > > > > Also, observing size change without observing mtime change > > > after crash seems like a very bad outcome that may be possible > > > after your change. > > > > > > These are just a few cases that I could think of, they may be filesystem > > > dependent, but my gut feeling is that if you remove the time update before > > > the operation, that has been like that forever, a lot of s#!t is going to float > > > for various filesystems and applications. > > > > > > And it is not one of those things that are discovered during rc or even > > > stable kernel testing - they are discovered much later when users start to > > > realize their applications got bogged up after crash, so it feels like to me > > > like playing with fire. > > > > > > > > If I read the problem description correctly, then a solution that invalidates > > > > > the NFS cache before AND after the write would be acceptable. Right? > > > > > Would an extra i_version bump after the write solve the race? > > > > > > > > > > > > > I based this patch on Neil's assertion that updating the time before an > > > > operation was pointless if we were going to do it afterward. The NFS > > > > client only really cares about seeing it change after a write. > > > > > > > > > > Pointless to NFS client maybe. > > > Whether or not this is not changing user behavior for other applications > > > is up to you to prove and I doubt that you can prove it because I doubt > > > that it is true. > > > > > > > Doing both would be fine from a correctness standpoint, and in most > > > > cases, the second would be a no-op anyway since a query would have to > > > > race in between the two for that to happen. > > > > > > > > FWIW, I think we should update the m/ctime and version at the same time. > > > > If the version changes, then there is always the potential that a timer > > > > tick has occurred. So, that would translate to a second call to > > > > file_update_time in here. > > > > > > > > The downside of bumping the times/version both before and after is that > > > > these are hot codepaths, and we'd be adding extra operations there. Even > > > > in the case where nothing has changed, we'd have to call > > > > inode_needs_update_time a second time for every write. Is that worth the > > > > cost? > > > > > > Is there a practical cost for iversion bump AFTER write as I suggested? > > > If you NEED m/ctime update AFTER write and iversion update is not enough > > > then I did not understand from your commit message why that is. > > > > > > Thanks, > > > Amir. > > > > > > > Maybe we should split i_version updates from ctime updates. > > > > While it isn't true that ctime updates have happened before the write > > "forever" it has been true since 2.3.43[1] which is close to forever. > > > > For ctime there doesn't appear to be a strong specification of when the > > change happens, so history provides a good case for leaving it before. > > For i_version we want to provide clear and unambiguous semantics. > > Performing 2 updates makes the specification muddy. > > > > So I would prefer a single update for i_version, performed after the > > change becomes visible. If that means it has to be separate from ctime, > > then so be it. > > > > NeilBrown > > > > > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=636b38438001a00b25f23e38747a91cb8428af29 > > > Not necessarily. We can document it in such a way that bumping it twice > is allowed, but not required. > > My main concern with splitting them up is that we'd have to dirty the > inode twice if both the times and the i_version need updating. If the > inode gets written out in between, then we end up doing twice the I/O. > The interim on-disk metadata would be in sort of a weird state too -- > the ctime would have changed but the version would still be old. > > It might be worthwhile to just go ahead and continue bumping it in > file_update_time, and then we'd just attempt to bump the i_version again > afterward. The second bump will almost always be a no-op anyway. I"m probably starting to sound like a scratched record here, but this is why I think it should be up to the filesystem to bump i_version when it determines that it should. It should be in a position to include the i_version update any time that it writes the inode and so avoid a double write. Having that vfs/mm do so much of the work makes it hard for the filesystem to do the right amount of work. The common code should provide libraries of useful code, the filesystems should call that as appropriate. Some of our code is structured that way, some of it isn't. Most callers of file_update_time() are inside filesystems and that is good - they are in control. There are 3 in mm/*.c. Those are all in callbacks from the filesystem, so the fs could avoid them, but only by duplicating lots of code to avoid using the callback. Instead these file_update_time() calls should become more explicit calls into the filesystem telling the filesystem what has just happened, or is about to happen. Then the filesystem can do the right thing, rather than having something done to it. See also https://lwn.net/Articles/336262/ and the "midlayer mistake". But yes, doing the bump afterwards as well is likely to be a no-op most of the time and is probably the easy solution. Ugly, but easy. NeilBrown
diff --git a/mm/filemap.c b/mm/filemap.c index 15800334147b..72c0ceb75176 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -3812,10 +3812,6 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from) if (err) goto out; - err = file_update_time(file); - if (err) - goto out; - if (iocb->ki_flags & IOCB_DIRECT) { loff_t pos, endbyte; @@ -3868,6 +3864,19 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from) iocb->ki_pos += written; } out: + if (written > 0) { + err = file_update_time(file); + /* + * There isn't much we can do at this point if updating the + * times fails after a successful write. The times and i_version + * should still be updated in the inode, and it should still be + * marked dirty, so hopefully the next inode update will catch it. + * Log a warning once so we have a record that something untoward + * has occurred. + */ + WARN_ONCE(err, "Failed to update m/ctime after write: %ld\n", err); + } + current->backing_dev_info = NULL; return written ? written : err; }
The c/mtime and i_version currently get updated before the data is copied (or a DIO write is issued), which is problematic for NFS. READ+GETATTR can race with a write (even a local one) in such a way as to make the client associate the state of the file with the wrong change attribute. That association can persist indefinitely if the file sees no further changes. Move the setting of times to the bottom of the function in __generic_file_write_iter and only update it if something was successfully written. If the time update fails, log a warning once, but don't fail the write. All of the existing callers use update_time functions that don't fail, so we should never trip this. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- mm/filemap.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)