From: Jan Kara <jack@suse.cz>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: matthew.garrett@nebula.com, yuchao0@huawei.com, tytso@mit.edu,
ard.biesheuvel@linaro.org, josef@toxicpanda.com, clm@fb.com,
adilger.kernel@dilger.ca, viro@zeniv.linux.org.uk, jack@suse.com,
dsterba@suse.com, jaegeuk@kernel.org, jk@ozlabs.org,
reiserfs-devel@vger.kernel.org, linux-efi@vger.kernel.org,
devel@lists.orangefs.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
linux-xfs@vger.kernel.org, linux-mm@kvack.org,
linux-nilfs@vger.kernel.org, linux-mtd@lists.infradead.org,
ocfs2-devel@oss.oracle.com, linux-fsdevel@vger.kernel.org,
linux-ext4@vger.kernel.org, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 2/7] vfs: flush and wait for io when setting the immutable flag via SETFLAGS
Date: Mon, 24 Jun 2019 13:37:37 +0200 [thread overview]
Message-ID: <20190624113737.GG32376@quack2.suse.cz> (raw)
In-Reply-To: <156116142734.1664939.5074567130774423066.stgit@magnolia>
On Fri 21-06-19 16:57:07, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> When we're using FS_IOC_SETFLAGS to set the immutable flag on a file, we
> need to ensure that userspace can't continue to write the file after the
> file becomes immutable. To make that happen, we have to flush all the
> dirty pagecache pages to disk to ensure that we can fail a page fault on
> a mmap'd region, wait for pending directio to complete, and hope the
> caller locked out any new writes by holding the inode lock.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Seeing the way this worked out, is there a reason to have separate
vfs_ioc_setflags_flush_data() instead of folding the functionality in
vfs_ioc_setflags_check() (possibly renaming it to
vfs_ioc_setflags_prepare() to indicate it does already some changes)? I
don't see any place that would need these two separated...
> +/*
> + * Flush all pending IO and dirty mappings before setting S_IMMUTABLE on an
> + * inode via FS_IOC_SETFLAGS. If the flush fails we'll clear the flag before
> + * returning error.
> + *
> + * Note: the caller should be holding i_mutex, or else be sure that
> + * they have exclusive access to the inode structure.
> + */
> +static inline int vfs_ioc_setflags_flush_data(struct inode *inode, int flags)
> +{
> + int ret;
> +
> + if (!vfs_ioc_setflags_need_flush(inode, flags))
> + return 0;
> +
> + inode_set_flags(inode, S_IMMUTABLE, S_IMMUTABLE);
> + ret = inode_flush_data(inode);
> + if (ret)
> + inode_set_flags(inode, 0, S_IMMUTABLE);
> + return ret;
> +}
Also this sets S_IMMUTABLE whenever vfs_ioc_setflags_need_flush() returns
true. That is currently the right thing but seems like a landmine waiting
to trip? So I'd just drop the vfs_ioc_setflags_need_flush() abstraction to
make it clear what's going on.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
WARNING: multiple messages have this Message-ID (diff)
From: Jan Kara <jack@suse.cz>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: matthew.garrett@nebula.com, yuchao0@huawei.com, tytso@mit.edu,
ard.biesheuvel@linaro.org, josef@toxicpanda.com, clm@fb.com,
adilger.kernel@dilger.ca, viro@zeniv.linux.org.uk, jack@suse.com,
dsterba@suse.com, jaegeuk@kernel.org, jk@ozlabs.org,
reiserfs-devel@vger.kernel.org, linux-efi@vger.kernel.org,
devel@lists.orangefs.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
linux-xfs@vger.kernel.org, linux-mm@kvack.org,
linux-nilfs@vger.kernel.org, linux-mtd@lists.infradead.org,
ocfs2-devel@oss.oracle.com, linux-fsdevel@vger.kernel.org,
linux-ext4@vger.kernel.org, linux-btrfs@vger.kernel.org
Subject: [Ocfs2-devel] [PATCH 2/7] vfs: flush and wait for io when setting the immutable flag via SETFLAGS
Date: Mon, 24 Jun 2019 13:37:37 +0200 [thread overview]
Message-ID: <20190624113737.GG32376@quack2.suse.cz> (raw)
In-Reply-To: <156116142734.1664939.5074567130774423066.stgit@magnolia>
On Fri 21-06-19 16:57:07, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> When we're using FS_IOC_SETFLAGS to set the immutable flag on a file, we
> need to ensure that userspace can't continue to write the file after the
> file becomes immutable. To make that happen, we have to flush all the
> dirty pagecache pages to disk to ensure that we can fail a page fault on
> a mmap'd region, wait for pending directio to complete, and hope the
> caller locked out any new writes by holding the inode lock.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Seeing the way this worked out, is there a reason to have separate
vfs_ioc_setflags_flush_data() instead of folding the functionality in
vfs_ioc_setflags_check() (possibly renaming it to
vfs_ioc_setflags_prepare() to indicate it does already some changes)? I
don't see any place that would need these two separated...
> +/*
> + * Flush all pending IO and dirty mappings before setting S_IMMUTABLE on an
> + * inode via FS_IOC_SETFLAGS. If the flush fails we'll clear the flag before
> + * returning error.
> + *
> + * Note: the caller should be holding i_mutex, or else be sure that
> + * they have exclusive access to the inode structure.
> + */
> +static inline int vfs_ioc_setflags_flush_data(struct inode *inode, int flags)
> +{
> + int ret;
> +
> + if (!vfs_ioc_setflags_need_flush(inode, flags))
> + return 0;
> +
> + inode_set_flags(inode, S_IMMUTABLE, S_IMMUTABLE);
> + ret = inode_flush_data(inode);
> + if (ret)
> + inode_set_flags(inode, 0, S_IMMUTABLE);
> + return ret;
> +}
Also this sets S_IMMUTABLE whenever vfs_ioc_setflags_need_flush() returns
true. That is currently the right thing but seems like a landmine waiting
to trip? So I'd just drop the vfs_ioc_setflags_need_flush() abstraction to
make it clear what's going on.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
WARNING: multiple messages have this Message-ID (diff)
From: Jan Kara <jack@suse.cz>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-efi@vger.kernel.org, linux-btrfs@vger.kernel.org,
linux-mm@kvack.org, clm@fb.com, adilger.kernel@dilger.ca,
matthew.garrett@nebula.com, linux-nilfs@vger.kernel.org,
linux-ext4@vger.kernel.org, devel@lists.orangefs.org,
josef@toxicpanda.com, reiserfs-devel@vger.kernel.org,
viro@zeniv.linux.org.uk, dsterba@suse.com, jaegeuk@kernel.org,
tytso@mit.edu, ard.biesheuvel@linaro.org,
linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
linux-xfs@vger.kernel.org, jk@ozlabs.org, jack@suse.com,
linux-fsdevel@vger.kernel.org, linux-mtd@lists.infradead.org,
ocfs2-devel@oss.oracle.com
Subject: Re: [f2fs-dev] [PATCH 2/7] vfs: flush and wait for io when setting the immutable flag via SETFLAGS
Date: Mon, 24 Jun 2019 13:37:37 +0200 [thread overview]
Message-ID: <20190624113737.GG32376@quack2.suse.cz> (raw)
In-Reply-To: <156116142734.1664939.5074567130774423066.stgit@magnolia>
On Fri 21-06-19 16:57:07, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> When we're using FS_IOC_SETFLAGS to set the immutable flag on a file, we
> need to ensure that userspace can't continue to write the file after the
> file becomes immutable. To make that happen, we have to flush all the
> dirty pagecache pages to disk to ensure that we can fail a page fault on
> a mmap'd region, wait for pending directio to complete, and hope the
> caller locked out any new writes by holding the inode lock.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Seeing the way this worked out, is there a reason to have separate
vfs_ioc_setflags_flush_data() instead of folding the functionality in
vfs_ioc_setflags_check() (possibly renaming it to
vfs_ioc_setflags_prepare() to indicate it does already some changes)? I
don't see any place that would need these two separated...
> +/*
> + * Flush all pending IO and dirty mappings before setting S_IMMUTABLE on an
> + * inode via FS_IOC_SETFLAGS. If the flush fails we'll clear the flag before
> + * returning error.
> + *
> + * Note: the caller should be holding i_mutex, or else be sure that
> + * they have exclusive access to the inode structure.
> + */
> +static inline int vfs_ioc_setflags_flush_data(struct inode *inode, int flags)
> +{
> + int ret;
> +
> + if (!vfs_ioc_setflags_need_flush(inode, flags))
> + return 0;
> +
> + inode_set_flags(inode, S_IMMUTABLE, S_IMMUTABLE);
> + ret = inode_flush_data(inode);
> + if (ret)
> + inode_set_flags(inode, 0, S_IMMUTABLE);
> + return ret;
> +}
Also this sets S_IMMUTABLE whenever vfs_ioc_setflags_need_flush() returns
true. That is currently the right thing but seems like a landmine waiting
to trip? So I'd just drop the vfs_ioc_setflags_need_flush() abstraction to
make it clear what's going on.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
WARNING: multiple messages have this Message-ID (diff)
From: Jan Kara <jack@suse.cz>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-efi@vger.kernel.org, linux-btrfs@vger.kernel.org,
yuchao0@huawei.com, linux-mm@kvack.org, clm@fb.com,
adilger.kernel@dilger.ca, matthew.garrett@nebula.com,
linux-nilfs@vger.kernel.org, linux-ext4@vger.kernel.org,
devel@lists.orangefs.org, josef@toxicpanda.com,
reiserfs-devel@vger.kernel.org, viro@zeniv.linux.org.uk,
dsterba@suse.com, jaegeuk@kernel.org, tytso@mit.edu,
ard.biesheuvel@linaro.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net,
linux-xfs@vger.kernel.org, jk@ozlabs.org, jack@suse.com,
linux-fsdevel@vger.kernel.org, linux-mtd@lists.infradead.org,
ocfs2-devel@oss.oracle.com
Subject: Re: [PATCH 2/7] vfs: flush and wait for io when setting the immutable flag via SETFLAGS
Date: Mon, 24 Jun 2019 13:37:37 +0200 [thread overview]
Message-ID: <20190624113737.GG32376@quack2.suse.cz> (raw)
In-Reply-To: <156116142734.1664939.5074567130774423066.stgit@magnolia>
On Fri 21-06-19 16:57:07, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> When we're using FS_IOC_SETFLAGS to set the immutable flag on a file, we
> need to ensure that userspace can't continue to write the file after the
> file becomes immutable. To make that happen, we have to flush all the
> dirty pagecache pages to disk to ensure that we can fail a page fault on
> a mmap'd region, wait for pending directio to complete, and hope the
> caller locked out any new writes by holding the inode lock.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Seeing the way this worked out, is there a reason to have separate
vfs_ioc_setflags_flush_data() instead of folding the functionality in
vfs_ioc_setflags_check() (possibly renaming it to
vfs_ioc_setflags_prepare() to indicate it does already some changes)? I
don't see any place that would need these two separated...
> +/*
> + * Flush all pending IO and dirty mappings before setting S_IMMUTABLE on an
> + * inode via FS_IOC_SETFLAGS. If the flush fails we'll clear the flag before
> + * returning error.
> + *
> + * Note: the caller should be holding i_mutex, or else be sure that
> + * they have exclusive access to the inode structure.
> + */
> +static inline int vfs_ioc_setflags_flush_data(struct inode *inode, int flags)
> +{
> + int ret;
> +
> + if (!vfs_ioc_setflags_need_flush(inode, flags))
> + return 0;
> +
> + inode_set_flags(inode, S_IMMUTABLE, S_IMMUTABLE);
> + ret = inode_flush_data(inode);
> + if (ret)
> + inode_set_flags(inode, 0, S_IMMUTABLE);
> + return ret;
> +}
Also this sets S_IMMUTABLE whenever vfs_ioc_setflags_need_flush() returns
true. That is currently the right thing but seems like a landmine waiting
to trip? So I'd just drop the vfs_ioc_setflags_need_flush() abstraction to
make it clear what's going on.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2019-06-24 11:37 UTC|newest]
Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-21 23:56 [PATCH v4 0/7] vfs: make immutable files actually immutable Darrick J. Wong
2019-06-21 23:56 ` Darrick J. Wong
2019-06-21 23:56 ` Darrick J. Wong
2019-06-21 23:56 ` [f2fs-dev] " Darrick J. Wong
2019-06-21 23:56 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-21 23:56 ` [PATCH 1/7] mm/fs: don't allow writes to immutable files Darrick J. Wong
2019-06-21 23:56 ` Darrick J. Wong
2019-06-21 23:56 ` Darrick J. Wong
2019-06-21 23:56 ` [f2fs-dev] " Darrick J. Wong
2019-06-21 23:56 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-24 11:13 ` Jan Kara
2019-06-24 11:13 ` Jan Kara
2019-06-24 11:13 ` [f2fs-dev] " Jan Kara
2019-06-24 11:13 ` [Ocfs2-devel] " Jan Kara
2019-06-21 23:57 ` [PATCH 2/7] vfs: flush and wait for io when setting the immutable flag via SETFLAGS Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` [f2fs-dev] " Darrick J. Wong
2019-06-21 23:57 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-24 11:37 ` Jan Kara [this message]
2019-06-24 11:37 ` Jan Kara
2019-06-24 11:37 ` [f2fs-dev] " Jan Kara
2019-06-24 11:37 ` [Ocfs2-devel] " Jan Kara
2019-06-24 21:58 ` Darrick J. Wong
2019-06-24 21:58 ` Darrick J. Wong
2019-06-24 21:58 ` [f2fs-dev] " Darrick J. Wong
2019-06-24 21:58 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-25 3:04 ` Darrick J. Wong
2019-06-25 3:04 ` Darrick J. Wong
2019-06-25 3:04 ` [f2fs-dev] " Darrick J. Wong
2019-06-25 3:04 ` Darrick J. Wong
2019-06-25 7:08 ` Jan Kara
2019-06-25 7:08 ` Jan Kara
2019-06-25 7:08 ` [f2fs-dev] " Jan Kara
2019-06-25 7:08 ` Jan Kara
2019-06-24 15:33 ` Jan Kara
2019-06-24 15:33 ` Jan Kara
2019-06-24 15:33 ` [f2fs-dev] " Jan Kara
2019-06-24 15:33 ` [Ocfs2-devel] " Jan Kara
2019-06-24 16:36 ` Darrick J. Wong
2019-06-24 16:36 ` Darrick J. Wong
2019-06-24 16:36 ` [f2fs-dev] " Darrick J. Wong
2019-06-24 16:36 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-21 23:57 ` [PATCH 3/7] vfs: flush and wait for io when setting the immutable flag via FSSETXATTR Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` [f2fs-dev] " Darrick J. Wong
2019-06-21 23:57 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-21 23:57 ` [PATCH 4/7] vfs: don't allow most setxattr to immutable files Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` [f2fs-dev] " Darrick J. Wong
2019-06-21 23:57 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-21 23:57 ` [PATCH 5/7] xfs: refactor setflags to use setattr code directly Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` [f2fs-dev] " Darrick J. Wong
2019-06-21 23:57 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-21 23:57 ` [PATCH 6/7] xfs: clean up xfs_merge_ioc_xflags Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` [f2fs-dev] " Darrick J. Wong
2019-06-21 23:57 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-21 23:57 ` [PATCH 7/7] vfs: don't allow writes to swap files Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` Darrick J. Wong
2019-06-21 23:57 ` [f2fs-dev] " Darrick J. Wong
2019-06-21 23:57 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-25 10:36 ` [PATCH v4 0/7] vfs: make immutable files actually immutable Christoph Hellwig
2019-06-25 10:36 ` Christoph Hellwig
2019-06-25 10:36 ` [f2fs-dev] " Christoph Hellwig
2019-06-25 10:36 ` [Ocfs2-devel] " Christoph Hellwig
2019-06-25 18:03 ` Darrick J. Wong
2019-06-25 18:03 ` Darrick J. Wong
2019-06-25 18:03 ` [f2fs-dev] " Darrick J. Wong
2019-06-25 18:03 ` [Ocfs2-devel] " Darrick J. Wong
2019-06-25 20:37 ` Andreas Dilger
2019-06-25 20:37 ` Andreas Dilger
2019-06-25 20:37 ` Andreas Dilger
2019-06-25 20:37 ` [Ocfs2-devel] " Andreas Dilger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190624113737.GG32376@quack2.suse.cz \
--to=jack@suse.cz \
--cc=adilger.kernel@dilger.ca \
--cc=ard.biesheuvel@linaro.org \
--cc=clm@fb.com \
--cc=darrick.wong@oracle.com \
--cc=devel@lists.orangefs.org \
--cc=dsterba@suse.com \
--cc=jack@suse.com \
--cc=jaegeuk@kernel.org \
--cc=jk@ozlabs.org \
--cc=josef@toxicpanda.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=linux-efi@vger.kernel.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-mtd@lists.infradead.org \
--cc=linux-nilfs@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=matthew.garrett@nebula.com \
--cc=ocfs2-devel@oss.oracle.com \
--cc=reiserfs-devel@vger.kernel.org \
--cc=tytso@mit.edu \
--cc=viro@zeniv.linux.org.uk \
--cc=yuchao0@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.