Re: [f2fs-dev] [PATCH v10 43/62] memfd: Convert shmem_tag_pins to XArray
On Fri, Mar 30, 2018 at 05:05:05PM -0700, Mike Kravetz wrote: > On 03/29/2018 08:42 PM, Matthew Wilcox wrote: > > From: Matthew Wilcox > > > > Simplify the locking by taking the spinlock while we walk the tree on > > the assumption that many acquires and releases of the lock will be > > worse than holding the lock for a (potentially) long time. > > I see this change made in several of the patches and do not have a > specific issue with it. As part of the XArray implementation you > have XA_CHECK_SCHED = 4096. So, we drop locks and do a cond_resched > after XA_CHECK_SCHED iterations. Just curious how you came up with > that number. Apologies in advance if this was discussed in a previous > round of reviews. It comes from two places, the current implementations of tag_pages_for_writeback() and find_swap_entry(). I have no idea if it's the optimal number for anybody, but it's the only number that anyone was using. I'll have no problem if somebody suggests we tweak the number in the future. > I did not do a detailed review of the XArray implementation. Only looked > at the provided interfaces and their intended uses. If the interfaces work > as specified, the changes here are fine. > > Reviewed-by: Mike Kravetz That is all I can ask for, and it's the most valuable thing. Thanks, Mike! -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs: truncate preallocated blocks in error case
If write is failed, we must deallocate the blocks that we couldn't write.
Cc: sta...@vger.kernel.org
Signed-off-by: Jaegeuk Kim
---
fs/f2fs/file.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 8068b015ece5..f18f62dd60a3 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2911,6 +2911,8 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb,
struct iov_iter *from)
ret = generic_write_checks(iocb, from);
if (ret > 0) {
+ bool preallocated = false;
+ size_t target_size;
int err;
if (iov_iter_fault_in_readable(from, iov_iter_count(from)))
@@ -2927,6 +2929,9 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb,
struct iov_iter *from)
}
} else {
+ preallocated = true;
+ target_size = iocb->ki_pos + iov_iter_count(from);
+
err = f2fs_preallocate_blocks(iocb, from);
if (err) {
clear_inode_flag(inode, FI_NO_PREALLOC);
@@ -2939,6 +2944,10 @@ static ssize_t f2fs_file_write_iter(struct kiocb *iocb,
struct iov_iter *from)
blk_finish_plug(&plug);
clear_inode_flag(inode, FI_NO_PREALLOC);
+ /* if we couldn't write data, we should deallocate blocks. */
+ if (preallocated && i_size_read(inode) < target_size)
+ f2fs_truncate(inode);
+
if (ret > 0)
f2fs_update_iostat(F2FS_I_SB(inode), APP_WRITE_IO, ret);
}
--
2.15.0.531.g2ccb3012c9-goog
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 43/62] memfd: Convert shmem_tag_pins to XArray
On 03/29/2018 08:42 PM, Matthew Wilcox wrote:
> From: Matthew Wilcox
>
> Simplify the locking by taking the spinlock while we walk the tree on
> the assumption that many acquires and releases of the lock will be
> worse than holding the lock for a (potentially) long time.
I see this change made in several of the patches and do not have a
specific issue with it. As part of the XArray implementation you
have XA_CHECK_SCHED = 4096. So, we drop locks and do a cond_resched
after XA_CHECK_SCHED iterations. Just curious how you came up with
that number. Apologies in advance if this was discussed in a previous
round of reviews.
>
> We could replicate the same locking behaviour with the xarray, but would
> have to be careful that the xa_node wasn't RCU-freed under us before we
> took the lock.
>
> Signed-off-by: Matthew Wilcox
I did not do a detailed review of the XArray implementation. Only looked
at the provided interfaces and their intended uses. If the interfaces work
as specified, the changes here are fine.
Reviewed-by: Mike Kravetz
--
Mike Kravetz
> ---
> mm/memfd.c | 43 ++-
> 1 file changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 4cf7401cb09c..3b299d72df78 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -21,7 +21,7 @@
> #include
>
> /*
> - * We need a tag: a new tag would expand every radix_tree_node by 8 bytes,
> + * We need a tag: a new tag would expand every xa_node by 8 bytes,
> * so reuse a tag which we firmly believe is never set or cleared on shmem.
> */
> #define SHMEM_TAG_PINNEDPAGECACHE_TAG_TOWRITE
> @@ -29,35 +29,28 @@
>
> static void shmem_tag_pins(struct address_space *mapping)
> {
> - struct radix_tree_iter iter;
> - void __rcu **slot;
> - pgoff_t start;
> + XA_STATE(xas, &mapping->i_pages, 0);
> struct page *page;
> + unsigned int tagged = 0;
>
> lru_add_drain();
> - start = 0;
> - rcu_read_lock();
> -
> - radix_tree_for_each_slot(slot, &mapping->i_pages, &iter, start) {
> - page = radix_tree_deref_slot(slot);
> - if (!page || radix_tree_exception(page)) {
> - if (radix_tree_deref_retry(page)) {
> - slot = radix_tree_iter_retry(&iter);
> - continue;
> - }
> - } else if (page_count(page) - page_mapcount(page) > 1) {
> - xa_lock_irq(&mapping->i_pages);
> - radix_tree_tag_set(&mapping->i_pages, iter.index,
> -SHMEM_TAG_PINNED);
> - xa_unlock_irq(&mapping->i_pages);
> - }
>
> - if (need_resched()) {
> - slot = radix_tree_iter_resume(slot, &iter);
> - cond_resched_rcu();
> - }
> + xas_lock_irq(&xas);
> + xas_for_each(&xas, page, ULONG_MAX) {
> + if (xa_is_value(page))
> + continue;
> + if (page_count(page) - page_mapcount(page) > 1)
> + xas_set_tag(&xas, SHMEM_TAG_PINNED);
> +
> + if (++tagged % XA_CHECK_SCHED)
> + continue;
> +
> + xas_pause(&xas);
> + xas_unlock_irq(&xas);
> + cond_resched();
> + xas_lock_irq(&xas);
> }
> - rcu_read_unlock();
> + xas_unlock_irq(&xas);
> }
>
> /*
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v10 44/62] memfd: Convert shmem_wait_for_pins to XArray
On 03/29/2018 08:42 PM, Matthew Wilcox wrote:
> From: Matthew Wilcox
>
> As with shmem_tag_pins(), hold the lock around the entire loop instead
> of acquiring & dropping it for each entry we're going to untag.
>
> Signed-off-by: Matthew Wilcox
Same comments as with with the previous shmem_tag_pins patch.
Reviewed-by: Mike Kravetz
--
Mike Kravetz
> ---
> mm/memfd.c | 61 +
> 1 file changed, 25 insertions(+), 36 deletions(-)
>
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 3b299d72df78..0e0835e63af2 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -64,9 +64,7 @@ static void shmem_tag_pins(struct address_space *mapping)
> */
> static int shmem_wait_for_pins(struct address_space *mapping)
> {
> - struct radix_tree_iter iter;
> - void __rcu **slot;
> - pgoff_t start;
> + XA_STATE(xas, &mapping->i_pages, 0);
> struct page *page;
> int error, scan;
>
> @@ -74,7 +72,9 @@ static int shmem_wait_for_pins(struct address_space
> *mapping)
>
> error = 0;
> for (scan = 0; scan <= LAST_SCAN; scan++) {
> - if (!radix_tree_tagged(&mapping->i_pages, SHMEM_TAG_PINNED))
> + unsigned int tagged = 0;
> +
> + if (!xas_tagged(&xas, SHMEM_TAG_PINNED))
> break;
>
> if (!scan)
> @@ -82,45 +82,34 @@ static int shmem_wait_for_pins(struct address_space
> *mapping)
> else if (schedule_timeout_killable((HZ << scan) / 200))
> scan = LAST_SCAN;
>
> - start = 0;
> - rcu_read_lock();
> - radix_tree_for_each_tagged(slot, &mapping->i_pages, &iter,
> -start, SHMEM_TAG_PINNED) {
> -
> - page = radix_tree_deref_slot(slot);
> - if (radix_tree_exception(page)) {
> - if (radix_tree_deref_retry(page)) {
> - slot = radix_tree_iter_retry(&iter);
> - continue;
> - }
> -
> - page = NULL;
> - }
> -
> - if (page &&
> - page_count(page) - page_mapcount(page) != 1) {
> - if (scan < LAST_SCAN)
> - goto continue_resched;
> -
> + xas_set(&xas, 0);
> + xas_lock_irq(&xas);
> + xas_for_each_tag(&xas, page, ULONG_MAX, SHMEM_TAG_PINNED) {
> + bool clear = true;
> + if (xa_is_value(page))
> + continue;
> + if (page_count(page) - page_mapcount(page) != 1) {
> /*
>* On the last scan, we clean up all those tags
>* we inserted; but make a note that we still
>* found pages pinned.
>*/
> - error = -EBUSY;
> - }
> -
> - xa_lock_irq(&mapping->i_pages);
> - radix_tree_tag_clear(&mapping->i_pages,
> - iter.index, SHMEM_TAG_PINNED);
> - xa_unlock_irq(&mapping->i_pages);
> -continue_resched:
> - if (need_resched()) {
> - slot = radix_tree_iter_resume(slot, &iter);
> - cond_resched_rcu();
> + if (scan == LAST_SCAN)
> + error = -EBUSY;
> + else
> + clear = false;
> }
> + if (clear)
> + xas_clear_tag(&xas, SHMEM_TAG_PINNED);
> + if (++tagged % XA_CHECK_SCHED)
> + continue;
> +
> + xas_pause(&xas);
> + xas_unlock_irq(&xas);
> + cond_resched();
> + xas_lock_irq(&xas);
> }
> - rcu_read_unlock();
> + xas_unlock_irq(&xas);
> }
>
> return error;
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: reserve bits for fs-verity
Hi Chao and Jaegeuk, On Fri, Mar 30, 2018 at 09:41:36AM -0700, Jaegeuk Kim wrote: > On 03/30, Chao Yu wrote: > > Hi Eric, > > > > On 2018/3/29 2:15, Eric Biggers wrote: > > > Reserve an F2FS feature flag and inode flag for fs-verity. This is an > > > in-development feature that is planned be discussed at LSF/MM 2018 [1]. > > > It will provide file-based integrity and authenticity for read-only > > > files. Most code will be in a filesystem-independent module, with > > > smaller changes needed to individual filesystems that opt-in to > > > supporting the feature. An early prototype supporting F2FS is available > > > [2]. Reserving the F2FS on-disk bits for fs-verity will prevent users > > > of the prototype from conflicting with other new F2FS features. > > > > > > Note that we're reserving the inode flag in f2fs_inode.i_advise, which > > > isn't really appropriate since it's not a hint or advice. But > > > ->i_advise is already being used to hold the 'encrypt' flag; and F2FS's > > > ->i_flags uses the generic FS_* values, so it seems ->i_flags can't be > > > used for an F2FS-specific flag without additional work to remove the > > > assumption that ->i_flags uses the generic flags namespace. > > > > At a glance, this is a VFS feature, can we search free slot, and define > > FS_VERITY_FL like other generic flags, so we can intergrate this flag into > > f2fs_inode::i_flags? > > Do we need to get/set this bit of i_flags to user? And, f2fs doesn't > synchronize > it with inode block update. I think this should be set by internal f2fs > operations likewise fscrypt. > The fs-verity inode flag won't be modifiable using FS_IOC_SETFLAGS. Like fscrypt, it will only be possible to set it using a dedicated ioctl (tentatively called FS_IOC_ENABLE_VERITY), and it won't be supported to clear the bit once set, short of deleting and re-creating the file. So it doesn't really matter where the bit goes in the on-disk inode, it just needs to go somewhere. I'm just hesitant to reserve a flag in the UAPI flags namespace which is really more "owned" by ext4 than by f2fs, so has more implications than just f2fs as we would effectively be reserving the flag for ext4's on-disk format too. I do think the flag *should* go in i_flags rather than i_advise, but I think the assumption that f2fs's inode flags namespace matches ext4's would first need to be removed so as to not tie the on-disk formats of different filesystems together. > > > > And how about applying this patch inside the patchset of new fsverity > > feature? > > Since once fsverity feature has some design modification, I worry about > > that may > > be we need to change this bit? result in disk layout incompatibility. > > The whole body is not fully mergeable, so once reserving the bits first, we > can > support it f2fs-tools and prepare the feature in advance. Based on previous > fscrypt experience, I don't expect we need to modify or drop these > dramatically > later. > > Any thoughts? > Yep, the full patchset isn't ready to be merged upstream yet. Other parts of the API/ABI such as the fs-verity metadata format, the ioctl numbers, and the semantics of accessing such files is subject to change. We know we'll need a superblock feature flag and a per-inode bit in any case, though. Personally I'd prefer to wait for the full patchset too, but we have people who want to start using the prototype of the feature already, so having f2fs-tools support the feature flag and having the bits not conflict with other f2fs features will be helpful. Thanks, Eric -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: reserve bits for fs-verity
On 03/30, Chao Yu wrote:
> Hi Eric,
>
> On 2018/3/29 2:15, Eric Biggers wrote:
> > Reserve an F2FS feature flag and inode flag for fs-verity. This is an
> > in-development feature that is planned be discussed at LSF/MM 2018 [1].
> > It will provide file-based integrity and authenticity for read-only
> > files. Most code will be in a filesystem-independent module, with
> > smaller changes needed to individual filesystems that opt-in to
> > supporting the feature. An early prototype supporting F2FS is available
> > [2]. Reserving the F2FS on-disk bits for fs-verity will prevent users
> > of the prototype from conflicting with other new F2FS features.
> >
> > Note that we're reserving the inode flag in f2fs_inode.i_advise, which
> > isn't really appropriate since it's not a hint or advice. But
> > ->i_advise is already being used to hold the 'encrypt' flag; and F2FS's
> > ->i_flags uses the generic FS_* values, so it seems ->i_flags can't be
> > used for an F2FS-specific flag without additional work to remove the
> > assumption that ->i_flags uses the generic flags namespace.
>
> At a glance, this is a VFS feature, can we search free slot, and define
> FS_VERITY_FL like other generic flags, so we can intergrate this flag into
> f2fs_inode::i_flags?
Do we need to get/set this bit of i_flags to user? And, f2fs doesn't synchronize
it with inode block update. I think this should be set by internal f2fs
operations likewise fscrypt.
>
> And how about applying this patch inside the patchset of new fsverity feature?
> Since once fsverity feature has some design modification, I worry about that
> may
> be we need to change this bit? result in disk layout incompatibility.
The whole body is not fully mergeable, so once reserving the bits first, we can
support it f2fs-tools and prepare the feature in advance. Based on previous
fscrypt experience, I don't expect we need to modify or drop these dramatically
later.
Any thoughts?
>
> Thanks,
>
> >
> > [1] https://marc.info/?l=linux-fsdevel&m=151690752225644
> > [2]
> > https://git.kernel.org/pub/scm/linux/kernel/git/mhalcrow/linux.git/log/?h=fs-verity-dev
> >
> > Signed-off-by: Eric Biggers
> > ---
> > fs/f2fs/f2fs.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index ae69dc358401..96d7809c4541 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -146,6 +146,7 @@ struct f2fs_mount_info {
> > #define F2FS_FEATURE_QUOTA_INO 0x0080
> > #define F2FS_FEATURE_INODE_CRTIME 0x0100
> > #define F2FS_FEATURE_LOST_FOUND0x0200
> > +#define F2FS_FEATURE_VERITY0x0400 /* reserved */
> >
> > #define F2FS_HAS_FEATURE(sb, mask) \
> > ((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
> > @@ -598,6 +599,7 @@ enum {
> > #define FADVISE_ENC_NAME_BIT 0x08
> > #define FADVISE_KEEP_SIZE_BIT 0x10
> > #define FADVISE_HOT_BIT0x20
> > +#define FADVISE_VERITY_BIT 0x40/* reserved */
> >
> > #define file_is_cold(inode)is_file(inode, FADVISE_COLD_BIT)
> > #define file_wrong_pino(inode) is_file(inode, FADVISE_LOST_PINO_BIT)
> >
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: remain written times to update inode during fsync
On 03/30, Chao Yu wrote:
> Hi Jaegeuk,
>
> On 2018/3/30 13:51, Jaegeuk Kim wrote:
> > This fixes xfstests/generic/392.
>
> Hmm... Could you please give more details about this issue and solution in
> commit message, since I can catch up the solution only with the code.
Yeah, I missed some explanation. :P
>
> Thanks,
>
> >
> > Signed-off-by: Jaegeuk Kim
> > ---
> > fs/f2fs/f2fs.h | 15 +++
> > fs/f2fs/inode.c | 4
> > 2 files changed, 19 insertions(+)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 000f93f6767e..675c39d85111 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -664,6 +664,7 @@ struct f2fs_inode_info {
> > kprojid_t i_projid; /* id for project quota */
> > int i_inline_xattr_size;/* inline xattr size */
> > struct timespec i_crtime; /* inode creation time */
> > + struct timespec i_disk_time[4]; /* inode disk times */
> > };
> >
> > static inline void get_extent_info(struct extent_info *ext,
> > @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode,
> > int type)
> > f2fs_mark_inode_dirty_sync(inode, true);
> > }
> >
> > +static inline bool time_equal(struct timespec a, struct timespec b)
> > +{
> > + return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
> > +}
> > +
> > static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
> > {
> > bool ret;
> > @@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct
> > inode *inode, int dsync)
> > i_size_read(inode) & ~PAGE_MASK)
> > return false;
> >
> > + if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
> > + return false;
> > + if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
> > + return false;
> > + if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
> > + return false;
> > + if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
> > + return false;
> > +
> > down_read(&F2FS_I(inode)->i_sem);
> > ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
> > up_read(&F2FS_I(inode)->i_sem);
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 401f09ccce7e..70aba580f4b0 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page
> > *node_page)
> > if (inode->i_nlink == 0)
> > clear_inline_node(node_page);
> >
> > + F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
> > + F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
> > + F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
> > + F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
> > }
> >
> > void update_inode_page(struct inode *inode)
> >
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v2] f2fs: remain written times to update inode during fsync
Change log from v1:
- add more description
This fixes xfstests/generic/392.
The failure was caused by different times between 1) one marked in the last
fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
The reason was that we skipped updating inode block at 1), since its i_size
was recoverable along with 4KB-aligned data writes, which was fixed by:
"f2fs: fix a wrong condition in f2fs_skip_inode_update"
Signed-off-by: Jaegeuk Kim
---
fs/f2fs/f2fs.h | 15 +++
fs/f2fs/inode.c | 4
2 files changed, 19 insertions(+)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 000f93f6767e..675c39d85111 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -664,6 +664,7 @@ struct f2fs_inode_info {
kprojid_t i_projid; /* id for project quota */
int i_inline_xattr_size;/* inline xattr size */
struct timespec i_crtime; /* inode creation time */
+ struct timespec i_disk_time[4]; /* inode disk times */
};
static inline void get_extent_info(struct extent_info *ext,
@@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int
type)
f2fs_mark_inode_dirty_sync(inode, true);
}
+static inline bool time_equal(struct timespec a, struct timespec b)
+{
+ return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
+}
+
static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
{
bool ret;
@@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode
*inode, int dsync)
i_size_read(inode) & ~PAGE_MASK)
return false;
+ if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
+ return false;
+ if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
+ return false;
+ if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
+ return false;
+ if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
+ return false;
+
down_read(&F2FS_I(inode)->i_sem);
ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
up_read(&F2FS_I(inode)->i_sem);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 401f09ccce7e..70aba580f4b0 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page
*node_page)
if (inode->i_nlink == 0)
clear_inline_node(node_page);
+ F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
+ F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
+ F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
+ F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
}
void update_inode_page(struct inode *inode)
--
2.15.0.531.g2ccb3012c9-goog
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] libf2fs: reset wanted_total_sectors by new sector_size
On 03/30, Jaegeuk Kim wrote:
> On 03/30, Junling Zheng wrote:
> > On 2018/3/30 19:26, Chao Yu wrote:
> > > On 2018/3/30 18:51, Junling Zheng wrote:
> > >> Hi,
> > >>
> > >> On 2018/3/30 17:28, Chao Yu wrote:
> > >>> Hi All,
> > >>>
> > >>> On 2018/3/28 1:19, Jaegeuk Kim wrote:
> > From: katao
> >
> > The args of wanted_total_sectors is calculated based
> > on the DEFAULT_SECTOR_SIZE(512Bytes).get_device_info(i)
> > may be reset dev_sector_size, we should reset the number
> > of wanted_total_sectors.
> >
> > This bug was reported to Google Issue Tracker.
> > Link: https://issuetracker.google.com/issues/76407663
> > >>>
> > >>> I don't think this is the right way, since now we have changed previous
> > >>> sector_counter's meaning, some applications, for example, like xfstests
> > >>> will get
> > >>> device's real sector size via blockdev --getsize64, then calculate
> > >>> total wanted
> > >>> sector count by total_wanted_size / real_sector_size, if we changed
> > >>> default
> > >>> sector size to 512bytes, xfstests will pass a wrong sector number,
> > >>> result in
> > >>> getting wrong partition size.
> > >>>
> > >>> For something worse, in order to get the correct sector number, we have
> > >>> to
> > >>> change the way of calculation method of xfstests for new mkfs, but how
> > >>> can
> > >>> xfstests know the current version of mkfs is new or old...
> > >>>
> > >>> I think the change didn't consider backward compatibility of mkfs, so,
> > >>> in order
> > >>> to keep that, we'd better to let user pass the right sector number
> > >>> based on
> > >>> their device, or we can introduce a new parameter to indicate user
> > >>> wanted total
> > >>> size.
> > >>>
> > >>> How do you think?
> > >>>
> > >>
> > >> Agree. It's not backward-compatible. Most users can pass the correct
> > >> sector number
> > >> calculated by the real sector size. For those very few users using 512B
> > >> despite of
> > >> the actual sector size, all we need to do is informing them the real
> > >> sector size.
> > >
> > > The problem is via passed sector number, we can't know user has already
> > > knew the
> > > real sector size or not, so we don't have any chance to info them.
> > >
> >
> > Yeah, we can't guess users' behaviors. And only when wanted size is over
> > device size,
> > we can inform users the real sector size.
>
> So, current problem would be that wanted_total_sectors is given by:
> - device sector size in xfstests
> - 4KB in fs_mgr in Android
> - 512B in recovery in Android
>
> And, my concern is why user always needs to think about sector_size to give
> target image size, and I thought 512B would be good to set as a default
> smallest
> value.
>
> Setting it 512B by default can give some confusion to users tho, it won't hurt
> the existing partitions or images. So, I bet users will get noticed quickly,
> when formatting new partition under this rule.
So, I just integrated two patches in terms of this issue. Could you take a
look at this?
>From 9e615dd0a350d336a8067c64d9ef2bb7ce43a3e5 Mon Sep 17 00:00:00 2001
From: katao
Date: Tue, 27 Mar 2018 13:25:46 +0800
Subject: [PATCH] libf2fs,mkfs.f2fs: reset wanted_total_sectors by new
sector_size
Use 512 bytes as the sector size criterion while specifying the
amount of sectors passed to mkfs.
Then, the args of wanted_total_sectors is calculated based
on the DEFAULT_SECTOR_SIZE(512Bytes). get_device_info(i)
may be reset dev_sector_size, we should reset the number
of wanted_total_sectors.
Signed-off-by: katao
Signed-off-by: Junling Zheng
Signed-off-by: Jaegeuk Kim
---
lib/libf2fs.c | 21 ++---
man/mkfs.f2fs.8 | 2 +-
mkfs/f2fs_format_main.c | 2 +-
3 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index ffdbccb..af1ca65 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -812,8 +812,21 @@ int get_device_info(int i)
#ifdef BLKSSZGET
if (ioctl(fd, BLKSSZGET, §or_size) < 0)
MSG(0, "\tError: Using the default sector size\n");
- else if (dev->sector_size < sector_size)
+ else if (dev->sector_size < sector_size){
+ /*
+* wanted_total_sectors need to be reset by new
+* sector_size.
+*/
+ if (c.wanted_total_sectors != -1) {
+ c.wanted_total_sectors =
+ (c.wanted_total_sectors *
+ dev->sector_size) / sector_size;
+ MSG(0, "Warn: change wanted sectors = %"PRIu64""
+ " (in %u bytes)\n",
+ c.wanted_total_sectors, sector_size);
+ }
dev->sector_size = sector_size;
+ }
#endif
Re: [f2fs-dev] [PATCH] libf2fs: reset wanted_total_sectors by new sector_size
On 03/30, Junling Zheng wrote:
> On 2018/3/30 19:26, Chao Yu wrote:
> > On 2018/3/30 18:51, Junling Zheng wrote:
> >> Hi,
> >>
> >> On 2018/3/30 17:28, Chao Yu wrote:
> >>> Hi All,
> >>>
> >>> On 2018/3/28 1:19, Jaegeuk Kim wrote:
> From: katao
>
> The args of wanted_total_sectors is calculated based
> on the DEFAULT_SECTOR_SIZE(512Bytes).get_device_info(i)
> may be reset dev_sector_size, we should reset the number
> of wanted_total_sectors.
>
> This bug was reported to Google Issue Tracker.
> Link: https://issuetracker.google.com/issues/76407663
> >>>
> >>> I don't think this is the right way, since now we have changed previous
> >>> sector_counter's meaning, some applications, for example, like xfstests
> >>> will get
> >>> device's real sector size via blockdev --getsize64, then calculate total
> >>> wanted
> >>> sector count by total_wanted_size / real_sector_size, if we changed
> >>> default
> >>> sector size to 512bytes, xfstests will pass a wrong sector number, result
> >>> in
> >>> getting wrong partition size.
> >>>
> >>> For something worse, in order to get the correct sector number, we have to
> >>> change the way of calculation method of xfstests for new mkfs, but how can
> >>> xfstests know the current version of mkfs is new or old...
> >>>
> >>> I think the change didn't consider backward compatibility of mkfs, so, in
> >>> order
> >>> to keep that, we'd better to let user pass the right sector number based
> >>> on
> >>> their device, or we can introduce a new parameter to indicate user wanted
> >>> total
> >>> size.
> >>>
> >>> How do you think?
> >>>
> >>
> >> Agree. It's not backward-compatible. Most users can pass the correct
> >> sector number
> >> calculated by the real sector size. For those very few users using 512B
> >> despite of
> >> the actual sector size, all we need to do is informing them the real
> >> sector size.
> >
> > The problem is via passed sector number, we can't know user has already
> > knew the
> > real sector size or not, so we don't have any chance to info them.
> >
>
> Yeah, we can't guess users' behaviors. And only when wanted size is over
> device size,
> we can inform users the real sector size.
So, current problem would be that wanted_total_sectors is given by:
- device sector size in xfstests
- 4KB in fs_mgr in Android
- 512B in recovery in Android
And, my concern is why user always needs to think about sector_size to give
target image size, and I thought 512B would be good to set as a default smallest
value.
Setting it 512B by default can give some confusion to users tho, it won't hurt
the existing partitions or images. So, I bet users will get noticed quickly,
when formatting new partition under this rule.
For xfstests, we're able to add another testcase in /f2fs to check this is
applied or not by simply testing some wanted_total_sectors numbers on
different sector sizes.
Thanks,
>
> > Thanks,
> >
> >>
> >> Thanks,
> >> Junling
> >>
> >>> Thanks,
> >>>
>
> Signed-off-by: katao
> Signed-off-by: Jaegeuk Kim
> ---
> lib/libf2fs.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
> index 0c684d5..5f11796 100644
> --- a/lib/libf2fs.c
> +++ b/lib/libf2fs.c
> @@ -799,8 +799,15 @@ int get_device_info(int i)
> #ifdef BLKSSZGET
> if (ioctl(fd, BLKSSZGET, §or_size) < 0)
> MSG(0, "\tError: Using the default sector
> size\n");
> -else if (dev->sector_size < sector_size)
> +else if (dev->sector_size < sector_size){
> +/*
> + * wanted_total_sectors need to be reset by new
> + * sector_size.
> + */
> +c.wanted_total_sectors =
> (c.wanted_total_sectors *
> +dev->sector_size) /
> sector_size;
> dev->sector_size = sector_size;
> +}
> #endif
> #ifdef BLKGETSIZE64
> if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {
>
> >>>
> >>>
> >>> .
> >>>
> >>
> >>
> >>
> >> .
> >>
> >
> >
> > .
> >
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 1/4] mkfs.f2fs: update extension lists
Hi Chao, > As I said, now we'd better to add a option in mkfs to disable default list > first. If you have time to work on this, I'm glad to review the patch.;) Yeah, I'll add that to my TODOs list :) > What I mean is we'd better to unify the list into one place A.. Now I understand why you had issues with the idea of kernelspace extension list. I've just checked the implementation of sysfs extension list update. I thought it'd be just a separate list handled within kernelspace f2fs volatile memory. Looks like it's an update to the existing superblock list, then you would be right, it'll get much more complicated. Now, the best idea I can come up with is: - Move the default list from mkfs.f2fs to kernelspace and remove the list from mkfs. - Store user's added/removed extensions separately in the superblock. This will break compatibility with older f2fs and f2fs-tools, so I don't think this is a good idea. Can you come up with a better idea? In summary, we need: - Always up-to-date default list, not fixed at the time of mkfs - Maintaining compatibility with different f2fs and f2fs-tools versions - Respecting user's choice to add or remove extensions Thanks. (This is more complicated than I originally anticipated, xD) On Fri, Mar 30, 2018 at 8:19 PM, Chao Yu wrote: > Hi Park, > > On 2018/3/29 23:54, Ju Hyung Park wrote: >> Hi Chao, >> >>> I think this is real hardcoded one >> >> Agreed, but I can't figure out a better way of doing this. >> I still don't think fixing it at mkfs point isn't a good idea. >> >> Doing this entirely on the userspace also won't make much sense since >> I would not trust the distros to ship "good extension lists". > > Hmm, if user can not trust these recommended extension list, we can introduce > mount option to let user to choose to disable all these default extensions, > and > do the configuration with -e to add their own extensions. Once they want to > update the list, sysfs entry will be recommended to make change. > >> >>> which will be hard to configure with if user >>> don't want this list at all. >> >> The new list is very conservative. >> Stuffs like .tar and .zip, which most users won't randomly write to, >> were excluded intentionally just by the fact that >> those are *capable* of random updates. >> Even compression methods like .bz2 were excluded since those are >> consisted of independent compressed blocks, >> which would mean random updates might be possible. >> >> I don't see why anyone would want to disable any of those, >> (even video editors won't be able to randomly write on mp4/mkv files) >> but they are still free to do so with the sysfs interface. > > Of course, but I doubt that in some private system, people may define .mp4 as > own extension of one kind of non-audio file, and do not need to add this type > extension, I think our filesystem should consider to allow user's custom > extension, instead of current hardcoded one. > > As I said, now we'd better to add a option in mkfs to disable default list > first. If you have time to work on this, I'm glad to review the patch.;) > >> >> The way I see it is that doing this in the kernel's default >> should be fine with almost every single users. >> >>> we have to use mount option >> >> Adding a mount option to bypass and disable >> any sort of kernelspace / mkfs list would be also nice. >> >>> We can add an new mkfs option to disable old common list, >> >> This would be also nice. >> The current code only appends user's list on the existing list. >> >>> then user can >>> enable/disable cold/hot file type as they prefer on mkfs/run-time. >> >> My point was that most users won't even know that the list has >> changed from f2fs-tools. >> Some users won't even aware of cold/hot concept in f2fs. >> If we do this from the kernel, just using the latest version of f2fs >> will bring their extension list up-to-date. >> >>> One question, how can you handle the conflict in between old list generated >>> during mkfs and new common list in run-time kernel? >> >> Valid point, but I think the added overhead of comparing both lists, >> possibly re-comparing the same extension, would be negligible. >> Can strcmp'ing ~70 entries be a considerable amount of overhead? > > What I mean is we'd better to unify the list into one place, now, I think > default place in super block is not bad. If we add one hardcoded list, we will > hard to know which one is deleted by user. > > Before: > thin: .mp4, .mp3... > > User disable .mp4 extension > thin list: .mp3... > .mp4 will not be treated as cold file. > > After: > thin: .mp4, .mp3... > rich: .mp4, .mp3... > > User disable .mp4 extension > thin list: .mp3... > rich list: .mp4, .mp3... > > .mp4 will still be treated as cold file, since it is in rich list > > Although, we can tag .mp4 is non-cold file in thin list, but after that, the > code logic become more complicate, and layout is be more mess. > > Thanks, > >> >> Thanks. >> >> On Thu, Mar 29, 2018 at 1:04 PM, Chao Y
Re: [f2fs-dev] [PATCH] libf2fs: reset wanted_total_sectors by new sector_size
On 2018/3/30 19:26, Chao Yu wrote:
> On 2018/3/30 18:51, Junling Zheng wrote:
>> Hi,
>>
>> On 2018/3/30 17:28, Chao Yu wrote:
>>> Hi All,
>>>
>>> On 2018/3/28 1:19, Jaegeuk Kim wrote:
From: katao
The args of wanted_total_sectors is calculated based
on the DEFAULT_SECTOR_SIZE(512Bytes).get_device_info(i)
may be reset dev_sector_size, we should reset the number
of wanted_total_sectors.
This bug was reported to Google Issue Tracker.
Link: https://issuetracker.google.com/issues/76407663
>>>
>>> I don't think this is the right way, since now we have changed previous
>>> sector_counter's meaning, some applications, for example, like xfstests
>>> will get
>>> device's real sector size via blockdev --getsize64, then calculate total
>>> wanted
>>> sector count by total_wanted_size / real_sector_size, if we changed default
>>> sector size to 512bytes, xfstests will pass a wrong sector number, result in
>>> getting wrong partition size.
>>>
>>> For something worse, in order to get the correct sector number, we have to
>>> change the way of calculation method of xfstests for new mkfs, but how can
>>> xfstests know the current version of mkfs is new or old...
>>>
>>> I think the change didn't consider backward compatibility of mkfs, so, in
>>> order
>>> to keep that, we'd better to let user pass the right sector number based on
>>> their device, or we can introduce a new parameter to indicate user wanted
>>> total
>>> size.
>>>
>>> How do you think?
>>>
>>
>> Agree. It's not backward-compatible. Most users can pass the correct sector
>> number
>> calculated by the real sector size. For those very few users using 512B
>> despite of
>> the actual sector size, all we need to do is informing them the real sector
>> size.
>
> The problem is via passed sector number, we can't know user has already knew
> the
> real sector size or not, so we don't have any chance to info them.
>
Yeah, we can't guess users' behaviors. And only when wanted size is over device
size,
we can inform users the real sector size.
> Thanks,
>
>>
>> Thanks,
>> Junling
>>
>>> Thanks,
>>>
Signed-off-by: katao
Signed-off-by: Jaegeuk Kim
---
lib/libf2fs.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index 0c684d5..5f11796 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -799,8 +799,15 @@ int get_device_info(int i)
#ifdef BLKSSZGET
if (ioctl(fd, BLKSSZGET, §or_size) < 0)
MSG(0, "\tError: Using the default sector size\n");
- else if (dev->sector_size < sector_size)
+ else if (dev->sector_size < sector_size){
+ /*
+ * wanted_total_sectors need to be reset by new
+ * sector_size.
+ */
+ c.wanted_total_sectors = (c.wanted_total_sectors *
+ dev->sector_size) / sector_size;
dev->sector_size = sector_size;
+ }
#endif
#ifdef BLKGETSIZE64
if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {
>>>
>>>
>>> .
>>>
>>
>>
>>
>> .
>>
>
>
> .
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] libf2fs: reset wanted_total_sectors by new sector_size
On 2018/3/30 18:51, Junling Zheng wrote:
> Hi,
>
> On 2018/3/30 17:28, Chao Yu wrote:
>> Hi All,
>>
>> On 2018/3/28 1:19, Jaegeuk Kim wrote:
>>> From: katao
>>>
>>> The args of wanted_total_sectors is calculated based
>>> on the DEFAULT_SECTOR_SIZE(512Bytes).get_device_info(i)
>>> may be reset dev_sector_size, we should reset the number
>>> of wanted_total_sectors.
>>>
>>> This bug was reported to Google Issue Tracker.
>>> Link: https://issuetracker.google.com/issues/76407663
>>
>> I don't think this is the right way, since now we have changed previous
>> sector_counter's meaning, some applications, for example, like xfstests will
>> get
>> device's real sector size via blockdev --getsize64, then calculate total
>> wanted
>> sector count by total_wanted_size / real_sector_size, if we changed default
>> sector size to 512bytes, xfstests will pass a wrong sector number, result in
>> getting wrong partition size.
>>
>> For something worse, in order to get the correct sector number, we have to
>> change the way of calculation method of xfstests for new mkfs, but how can
>> xfstests know the current version of mkfs is new or old...
>>
>> I think the change didn't consider backward compatibility of mkfs, so, in
>> order
>> to keep that, we'd better to let user pass the right sector number based on
>> their device, or we can introduce a new parameter to indicate user wanted
>> total
>> size.
>>
>> How do you think?
>>
>
> Agree. It's not backward-compatible. Most users can pass the correct sector
> number
> calculated by the real sector size. For those very few users using 512B
> despite of
> the actual sector size, all we need to do is informing them the real sector
> size.
The problem is via passed sector number, we can't know user has already knew the
real sector size or not, so we don't have any chance to info them.
Thanks,
>
> Thanks,
> Junling
>
>> Thanks,
>>
>>>
>>> Signed-off-by: katao
>>> Signed-off-by: Jaegeuk Kim
>>> ---
>>> lib/libf2fs.c | 9 -
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>>> index 0c684d5..5f11796 100644
>>> --- a/lib/libf2fs.c
>>> +++ b/lib/libf2fs.c
>>> @@ -799,8 +799,15 @@ int get_device_info(int i)
>>> #ifdef BLKSSZGET
>>> if (ioctl(fd, BLKSSZGET, §or_size) < 0)
>>> MSG(0, "\tError: Using the default sector size\n");
>>> - else if (dev->sector_size < sector_size)
>>> + else if (dev->sector_size < sector_size){
>>> + /*
>>> +* wanted_total_sectors need to be reset by new
>>> +* sector_size.
>>> +*/
>>> + c.wanted_total_sectors = (c.wanted_total_sectors *
>>> + dev->sector_size) / sector_size;
>>> dev->sector_size = sector_size;
>>> + }
>>> #endif
>>> #ifdef BLKGETSIZE64
>>> if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {
>>>
>>
>>
>> .
>>
>
>
>
> .
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 1/4] mkfs.f2fs: update extension lists
Hi Park, On 2018/3/29 23:54, Ju Hyung Park wrote: > Hi Chao, > >> I think this is real hardcoded one > > Agreed, but I can't figure out a better way of doing this. > I still don't think fixing it at mkfs point isn't a good idea. > > Doing this entirely on the userspace also won't make much sense since > I would not trust the distros to ship "good extension lists". Hmm, if user can not trust these recommended extension list, we can introduce mount option to let user to choose to disable all these default extensions, and do the configuration with -e to add their own extensions. Once they want to update the list, sysfs entry will be recommended to make change. > >> which will be hard to configure with if user >> don't want this list at all. > > The new list is very conservative. > Stuffs like .tar and .zip, which most users won't randomly write to, > were excluded intentionally just by the fact that > those are *capable* of random updates. > Even compression methods like .bz2 were excluded since those are > consisted of independent compressed blocks, > which would mean random updates might be possible. > > I don't see why anyone would want to disable any of those, > (even video editors won't be able to randomly write on mp4/mkv files) > but they are still free to do so with the sysfs interface. Of course, but I doubt that in some private system, people may define .mp4 as own extension of one kind of non-audio file, and do not need to add this type extension, I think our filesystem should consider to allow user's custom extension, instead of current hardcoded one. As I said, now we'd better to add a option in mkfs to disable default list first. If you have time to work on this, I'm glad to review the patch.;) > > The way I see it is that doing this in the kernel's default > should be fine with almost every single users. > >> we have to use mount option > > Adding a mount option to bypass and disable > any sort of kernelspace / mkfs list would be also nice. > >> We can add an new mkfs option to disable old common list, > > This would be also nice. > The current code only appends user's list on the existing list. > >> then user can >> enable/disable cold/hot file type as they prefer on mkfs/run-time. > > My point was that most users won't even know that the list has > changed from f2fs-tools. > Some users won't even aware of cold/hot concept in f2fs. > If we do this from the kernel, just using the latest version of f2fs > will bring their extension list up-to-date. > >> One question, how can you handle the conflict in between old list generated >> during mkfs and new common list in run-time kernel? > > Valid point, but I think the added overhead of comparing both lists, > possibly re-comparing the same extension, would be negligible. > Can strcmp'ing ~70 entries be a considerable amount of overhead? What I mean is we'd better to unify the list into one place, now, I think default place in super block is not bad. If we add one hardcoded list, we will hard to know which one is deleted by user. Before: thin: .mp4, .mp3... User disable .mp4 extension thin list: .mp3... .mp4 will not be treated as cold file. After: thin: .mp4, .mp3... rich: .mp4, .mp3... User disable .mp4 extension thin list: .mp3... rich list: .mp4, .mp3... .mp4 will still be treated as cold file, since it is in rich list Although, we can tag .mp4 is non-cold file in thin list, but after that, the code logic become more complicate, and layout is be more mess. Thanks, > > Thanks. > > On Thu, Mar 29, 2018 at 1:04 PM, Chao Yu wrote: >> Hi Park, >> >> On 2018/3/29 10:18, Ju Hyung Park wrote: >>> Hi Chao. >>> Do you mean we need add a new option to enable common list? >>> >>> No. I meant to move the list to the kernelspace's fs/f2fs or >>> include/linux/f2fs_fs.h. >> >> I think this is real hardcoded one, which will be hard to configure with if >> user >> don't want this list at all. How can you disable partial type in this common >> list? we have to use mount option or sysfs entry to do this at every mount >> time... >> >>> This eliminates the concern of the list getting too big and exceeding >>> the 64 limit, >>> and we can modify the list upon shipping new versions of f2fs. >>> >>> As I complained multiple times before, >>> I simply don't think that this list should be hardcoded at the time of mkfs. >>> >>> While users can still dynamically add/remove some extensions, >>> they can still have an older version of the "common list", >>> which then they have to use the sysfs interface everytime >>> whenever we update this list on mkfs.f2fs. >> >> We can add an new mkfs option to disable old common list, then user can >> enable/disable cold/hot file type as they prefer on mkfs/run-time. >> >>> >>> I'm not proposing to remove the list altogether in mkfs.f2fs. >>> I'm proposing to leave the old list in f2fs-tools, and write the new >>> list to the kernelspace code. >>> This way, older version of f2fs will still remain
Re: [f2fs-dev] [PATCH] f2fs: remain written times to update inode during fsync
Hi Jaegeuk,
On 2018/3/30 13:51, Jaegeuk Kim wrote:
> This fixes xfstests/generic/392.
Hmm... Could you please give more details about this issue and solution in
commit message, since I can catch up the solution only with the code.
Thanks,
>
> Signed-off-by: Jaegeuk Kim
> ---
> fs/f2fs/f2fs.h | 15 +++
> fs/f2fs/inode.c | 4
> 2 files changed, 19 insertions(+)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 000f93f6767e..675c39d85111 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -664,6 +664,7 @@ struct f2fs_inode_info {
> kprojid_t i_projid; /* id for project quota */
> int i_inline_xattr_size;/* inline xattr size */
> struct timespec i_crtime; /* inode creation time */
> + struct timespec i_disk_time[4]; /* inode disk times */
> };
>
> static inline void get_extent_info(struct extent_info *ext,
> @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int
> type)
> f2fs_mark_inode_dirty_sync(inode, true);
> }
>
> +static inline bool time_equal(struct timespec a, struct timespec b)
> +{
> + return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
> +}
> +
> static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
> {
> bool ret;
> @@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode
> *inode, int dsync)
> i_size_read(inode) & ~PAGE_MASK)
> return false;
>
> + if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
> + return false;
> + if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
> + return false;
> + if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
> + return false;
> + if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
> + return false;
> +
> down_read(&F2FS_I(inode)->i_sem);
> ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
> up_read(&F2FS_I(inode)->i_sem);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 401f09ccce7e..70aba580f4b0 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page
> *node_page)
> if (inode->i_nlink == 0)
> clear_inline_node(node_page);
>
> + F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
> + F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
> + F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
> + F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
> }
>
> void update_inode_page(struct inode *inode)
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] libf2fs: reset wanted_total_sectors by new sector_size
Hi,
On 2018/3/30 17:28, Chao Yu wrote:
> Hi All,
>
> On 2018/3/28 1:19, Jaegeuk Kim wrote:
>> From: katao
>>
>> The args of wanted_total_sectors is calculated based
>> on the DEFAULT_SECTOR_SIZE(512Bytes).get_device_info(i)
>> may be reset dev_sector_size, we should reset the number
>> of wanted_total_sectors.
>>
>> This bug was reported to Google Issue Tracker.
>> Link: https://issuetracker.google.com/issues/76407663
>
> I don't think this is the right way, since now we have changed previous
> sector_counter's meaning, some applications, for example, like xfstests will
> get
> device's real sector size via blockdev --getsize64, then calculate total
> wanted
> sector count by total_wanted_size / real_sector_size, if we changed default
> sector size to 512bytes, xfstests will pass a wrong sector number, result in
> getting wrong partition size.
>
> For something worse, in order to get the correct sector number, we have to
> change the way of calculation method of xfstests for new mkfs, but how can
> xfstests know the current version of mkfs is new or old...
>
> I think the change didn't consider backward compatibility of mkfs, so, in
> order
> to keep that, we'd better to let user pass the right sector number based on
> their device, or we can introduce a new parameter to indicate user wanted
> total
> size.
>
> How do you think?
>
Agree. It's not backward-compatible. Most users can pass the correct sector
number
calculated by the real sector size. For those very few users using 512B despite
of
the actual sector size, all we need to do is informing them the real sector
size.
Thanks,
Junling
> Thanks,
>
>>
>> Signed-off-by: katao
>> Signed-off-by: Jaegeuk Kim
>> ---
>> lib/libf2fs.c | 9 -
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>> index 0c684d5..5f11796 100644
>> --- a/lib/libf2fs.c
>> +++ b/lib/libf2fs.c
>> @@ -799,8 +799,15 @@ int get_device_info(int i)
>> #ifdef BLKSSZGET
>> if (ioctl(fd, BLKSSZGET, §or_size) < 0)
>> MSG(0, "\tError: Using the default sector size\n");
>> -else if (dev->sector_size < sector_size)
>> +else if (dev->sector_size < sector_size){
>> +/*
>> + * wanted_total_sectors need to be reset by new
>> + * sector_size.
>> + */
>> +c.wanted_total_sectors = (c.wanted_total_sectors *
>> +dev->sector_size) / sector_size;
>> dev->sector_size = sector_size;
>> +}
>> #endif
>> #ifdef BLKGETSIZE64
>> if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {
>>
>
>
> .
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: reserve bits for fs-verity
Hi Eric,
On 2018/3/29 2:15, Eric Biggers wrote:
> Reserve an F2FS feature flag and inode flag for fs-verity. This is an
> in-development feature that is planned be discussed at LSF/MM 2018 [1].
> It will provide file-based integrity and authenticity for read-only
> files. Most code will be in a filesystem-independent module, with
> smaller changes needed to individual filesystems that opt-in to
> supporting the feature. An early prototype supporting F2FS is available
> [2]. Reserving the F2FS on-disk bits for fs-verity will prevent users
> of the prototype from conflicting with other new F2FS features.
>
> Note that we're reserving the inode flag in f2fs_inode.i_advise, which
> isn't really appropriate since it's not a hint or advice. But
> ->i_advise is already being used to hold the 'encrypt' flag; and F2FS's
> ->i_flags uses the generic FS_* values, so it seems ->i_flags can't be
> used for an F2FS-specific flag without additional work to remove the
> assumption that ->i_flags uses the generic flags namespace.
At a glance, this is a VFS feature, can we search free slot, and define
FS_VERITY_FL like other generic flags, so we can intergrate this flag into
f2fs_inode::i_flags?
And how about applying this patch inside the patchset of new fsverity feature?
Since once fsverity feature has some design modification, I worry about that may
be we need to change this bit? result in disk layout incompatibility.
Thanks,
>
> [1] https://marc.info/?l=linux-fsdevel&m=151690752225644
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/mhalcrow/linux.git/log/?h=fs-verity-dev
>
> Signed-off-by: Eric Biggers
> ---
> fs/f2fs/f2fs.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index ae69dc358401..96d7809c4541 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -146,6 +146,7 @@ struct f2fs_mount_info {
> #define F2FS_FEATURE_QUOTA_INO 0x0080
> #define F2FS_FEATURE_INODE_CRTIME0x0100
> #define F2FS_FEATURE_LOST_FOUND 0x0200
> +#define F2FS_FEATURE_VERITY 0x0400 /* reserved */
>
> #define F2FS_HAS_FEATURE(sb, mask) \
> ((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
> @@ -598,6 +599,7 @@ enum {
> #define FADVISE_ENC_NAME_BIT 0x08
> #define FADVISE_KEEP_SIZE_BIT0x10
> #define FADVISE_HOT_BIT 0x20
> +#define FADVISE_VERITY_BIT 0x40/* reserved */
>
> #define file_is_cold(inode) is_file(inode, FADVISE_COLD_BIT)
> #define file_wrong_pino(inode) is_file(inode, FADVISE_LOST_PINO_BIT)
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] libf2fs: reset wanted_total_sectors by new sector_size
Hi All,
On 2018/3/28 1:19, Jaegeuk Kim wrote:
> From: katao
>
> The args of wanted_total_sectors is calculated based
> on the DEFAULT_SECTOR_SIZE(512Bytes).get_device_info(i)
> may be reset dev_sector_size, we should reset the number
> of wanted_total_sectors.
>
> This bug was reported to Google Issue Tracker.
> Link: https://issuetracker.google.com/issues/76407663
I don't think this is the right way, since now we have changed previous
sector_counter's meaning, some applications, for example, like xfstests will get
device's real sector size via blockdev --getsize64, then calculate total wanted
sector count by total_wanted_size / real_sector_size, if we changed default
sector size to 512bytes, xfstests will pass a wrong sector number, result in
getting wrong partition size.
For something worse, in order to get the correct sector number, we have to
change the way of calculation method of xfstests for new mkfs, but how can
xfstests know the current version of mkfs is new or old...
I think the change didn't consider backward compatibility of mkfs, so, in order
to keep that, we'd better to let user pass the right sector number based on
their device, or we can introduce a new parameter to indicate user wanted total
size.
How do you think?
Thanks,
>
> Signed-off-by: katao
> Signed-off-by: Jaegeuk Kim
> ---
> lib/libf2fs.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
> index 0c684d5..5f11796 100644
> --- a/lib/libf2fs.c
> +++ b/lib/libf2fs.c
> @@ -799,8 +799,15 @@ int get_device_info(int i)
> #ifdef BLKSSZGET
> if (ioctl(fd, BLKSSZGET, §or_size) < 0)
> MSG(0, "\tError: Using the default sector size\n");
> - else if (dev->sector_size < sector_size)
> + else if (dev->sector_size < sector_size){
> + /*
> + * wanted_total_sectors need to be reset by new
> + * sector_size.
> + */
> + c.wanted_total_sectors = (c.wanted_total_sectors *
> + dev->sector_size) / sector_size;
> dev->sector_size = sector_size;
> + }
> #endif
> #ifdef BLKGETSIZE64
> if (ioctl(fd, BLKGETSIZE64, &dev->total_sectors) < 0) {
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
