Re: [f2fs-dev] [PATCH] libf2fs: reset wanted_total_sectors by new sector_size
On 2018/4/3 4:03, Jaegeuk Kim wrote: > On 04/02, Chao Yu wrote: >> On 2018/3/30 23:39, 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. >> >> Of course, IMO, the most direct way is let user pass image size to mkfs.f2fs >> instead of caculated sector size, I think that will make all happy. > > No, it can become too large number. > >> >>> >>> 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 those f2fs users, who makes image based on non-512B sector device, once >> they >> upgrade mkfs.f2fs, they will encounter this issue, that may make bad >> reputation. >> >> For interface in f2fs private ioctl, sysfs entry, or syscall, we keep the >> rule >> that once we want to do some change on it, we will not change the old >> interface >> directly, instead, introduce a new one to keep backward compatibility of old >> one. Still, I hope we can keep that rule in mkfs.f2fs parameter. > > Okay, so in order to give more flexible ways, it'd be fine to add > wanted_sector_size by "-w" so that we could blow out all the existing concern > like this. That's better. > > From: katao > Date: Tue, 27 Mar 2018 13:25:46 +0800 > Subject: [PATCH] libf2fs,mkfs.f2fs: add wanted_sector_size for > wanted_total_sectors > > The wanted_total_sectors was determined by device sector size, but sometimes > we don't know precise sector_size by default. So, let's give > wanted_sector_size > in such the ambiguous situation. > > Signed-off-by: katao > Signed-off-by: Junling Zheng > Signed-off-by: Jaegeuk Kim Reviewed-by: Chao Yu Thanks, -- 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
On 04/03, Chao Yu wrote:
> On 2018/3/31 0:30, Jaegeuk Kim wrote:
> > 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);
> > +}
>
> We can use existing timespec_equal in instead of defining a
> duplicated one?
It is defined with const parameters.
>
> > +
> > 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;
>
> We need initialize them in do_read_inode?
Done.
Thanks,
>
> Thanks,
>
> > }
> >
> > 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 v3] f2fs: remain written times to update inode during fsync
Change log from v2:
- update do_read_inode
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 | 8
2 files changed, 23 insertions(+)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7102d3965fea..33be61b19a5f 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..e0d9e8f27ed2 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -284,6 +284,10 @@ static int do_read_inode(struct inode *inode)
fi->i_crtime.tv_nsec = le32_to_cpu(ri->i_crtime_nsec);
}
+ 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;
f2fs_put_page(node_page, 1);
stat_inc_inline_xattr(inode);
@@ -444,6 +448,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 1/4] mkfs.f2fs: update extension lists
Hi Park, On 2018/3/30 21:13, Ju Hyung Park wrote: > 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? Not yet, except supporting a optional full list in mkfs... :P > > 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 Yeah, I just think we need to simply extension list usage for user, also simply kernel/tools implementation. Thanks, > > 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: .m
Re: [f2fs-dev] [PATCH] f2fs: make assignment of t->dentry_bitmap more readable
On 2018/4/2 20:22, Yunlong Song wrote: > In make_dentry_ptr_block, it is confused with "&" for t->dentry_bitmap > but without "&" for t->dentry, so delete "&" to make code more readable. > > Signed-off-by: Yunlong Song Reviewed-by: Chao Yu Thanks, -- 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: truncate preallocated blocks in error case
On 2018/3/31 9:57, Jaegeuk Kim wrote: > If write is failed, we must deallocate the blocks that we couldn't write. > > Cc: sta...@vger.kernel.org > Signed-off-by: Jaegeuk Kim Reviewed-by: Chao Yu Thanks, -- 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
On 2018/3/31 0:30, Jaegeuk Kim wrote:
> 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);
> +}
We can use existing timespec_equal in instead of defining a
duplicated one?
> +
> 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;
We need initialize them in do_read_inode?
Thanks,
> }
>
> 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] mkfs.f2fs: support multiple features with one "-O"
Hi, Chao
On 2018/4/2 21:35, Chao Yu wrote:
> On 2018/4/2 12:19, Junling Zheng wrote:
>> Now one "-O" option can support multiple features separated
>> by a comma or blank, such as:
>> feature1,feature2,... or "feature1 feature2 ..."
>
> At a glance, can last sector number be parsed as feature name?
>
No, we should use quotation marks to wrap whole features while separating them
with blanks.
Thanks,
Junling
> Thanks,
>
>>
>> Signed-off-by: Junling Zheng
>> ---
>> mkfs/f2fs_format_main.c | 30 +++---
>> 1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/mkfs/f2fs_format_main.c b/mkfs/f2fs_format_main.c
>> index 741600e..76ff296 100644
>> --- a/mkfs/f2fs_format_main.c
>> +++ b/mkfs/f2fs_format_main.c
>> @@ -80,10 +80,8 @@ static void f2fs_show_info()
>> MSG(0, "Info: Trim is %s\n", c.trim ? "enabled": "disabled");
>> }
>>
>> -static void parse_feature(const char *features)
>> +static void set_feature_bits(char *features)
>> {
>> -while (*features == ' ')
>> -features++;
>> if (!strcmp(features, "encrypt")) {
>> c.feature |= cpu_to_le32(F2FS_FEATURE_ENCRYPT);
>> } else if (!strcmp(features, "verity")) {
>> @@ -108,6 +106,32 @@ static void parse_feature(const char *features)
>> }
>> }
>>
>> +static void parse_feature(const char *features)
>> +{
>> +char *buf, *sub, *next;
>> +
>> +buf = calloc(strlen(features) + 1, sizeof(char));
>> +ASSERT(buf);
>> +strncpy(buf, features, strlen(features) + 1);
>> +
>> +for (sub = buf; sub && *sub; sub = next ? next + 1 : NULL) {
>> +/* Skip the beginning blanks */
>> +while (*sub && *sub == ' ')
>> +sub++;
>> +next = sub;
>> +/* Skip a feature word */
>> +while (*next && *next != ' ' && *next != ',')
>> +next++;
>> +
>> +if (*next == 0)
>> +next = NULL;
>> +else
>> +*next = 0;
>> +
>> +set_feature_bits(sub);
>> +}
>> +}
>> +
>> static void f2fs_parse_options(int argc, char *argv[])
>> {
>> static const char *option_string = "qa:c:d:e:E:il:mo:O:s:S:z:t:f";
>>
>
> .
>
--
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 Tue, Apr 03, 2018 at 07:49:59AM +1000, Dave Chinner wrote:
> On Mon, Apr 02, 2018 at 11:48:37AM -0700, Eric Biggers wrote:
> > [+Cc linux-ext4, linux-fsdevel]
> >
> > On Mon, Apr 02, 2018 at 06:07:10PM +0800, Chao Yu wrote:
> > > Hi Eric and Jaegeuk,
> > >
> > > On 2018/3/31 2:34, Eric Biggers wrote:
> > > > 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
> > >
> > > Verity flag can also be wrapped by FS_FL_USER_MODIFIABLE like for
> > > FS_ENCRYPT_FL?
> > >
> > > > 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.
> > >
> > > IMO, because this is a VFS feature, it will be better that we can put it
> > > in more
> > > generic place, also user can check this bit in generic way (via
> > > FS_IOC_GETFLAGS), and then for other filesystem who wants add this
> > > feature, that
> > > will be simple to place this bit.
> > >
> > > What I can see is, for encryption feature:
> > >
> > > vfs::i_flags
> > > #define FS_ENCRYPT_FL 0x0800 /* Encrypted file */
> > >
> > > ext4:i_flags
> > > #define EXT4_ENCRYPT_FL 0x0800 /* encrypted file */
> > >
> > > f2fs::i_advice
> > > #define FADVISE_ENCRYPT_BIT 0x04
> > >
> > > It's very wired that f2fs didn't use well defined FS_ENCRYPT_FL bit
> > > position,
> > > result in that we leave a hole in on-disk i_flags, and if we want to show
> > > the
> > > same 'encrypted' flags status in FS_IOC_GETFLAGS, we need to change more
> > > codes.
> > >
> > > Anyway, I just ask why not let generic status goes into i_flags, and
> > > private
> > > status goes into i_advices?
> >
> > Ted and others, what would you say about allocating FS_VERITY_FL as one of
> > the
> > unused ext4 / "VFS" inode flags like 0x0100, or maybe 0x0400 if the
> > compression flags aren't being used anymore?
> >
> > I do wish that we separated the on-disk flags namespaces from the
> > FS_IOC_GETFLAGS/FS_IOC_SETFLAGS namespace though... Then adding the flag to
>
> Use FS_IOC_{GS}ETXATTR instead?. I'ts not tied to an on-disk format.
>
Yes, that API is better -- though, *setting* the fs-verity bit will likely be
done through its own ioctl (tentatively called FS_IOC_ENABLE_VERITY) as it will
involve more work than simply flipping the bit. So for *getting* it we maybe
should just add it as a statx attribute, and not use the GETFLAGS or GETXATTR
ioctls at all.
But right now the real problem is that f2fs is using the FS_* namespace for its
on-disk ->i_flags
Re: [f2fs-dev] [PATCH] f2fs: reserve bits for fs-verity
On Mon, Apr 02, 2018 at 11:48:37AM -0700, Eric Biggers wrote:
> [+Cc linux-ext4, linux-fsdevel]
>
> On Mon, Apr 02, 2018 at 06:07:10PM +0800, Chao Yu wrote:
> > Hi Eric and Jaegeuk,
> >
> > On 2018/3/31 2:34, Eric Biggers wrote:
> > > 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
> >
> > Verity flag can also be wrapped by FS_FL_USER_MODIFIABLE like for
> > FS_ENCRYPT_FL?
> >
> > > 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.
> >
> > IMO, because this is a VFS feature, it will be better that we can put it in
> > more
> > generic place, also user can check this bit in generic way (via
> > FS_IOC_GETFLAGS), and then for other filesystem who wants add this feature,
> > that
> > will be simple to place this bit.
> >
> > What I can see is, for encryption feature:
> >
> > vfs::i_flags
> > #define FS_ENCRYPT_FL 0x0800 /* Encrypted file */
> >
> > ext4:i_flags
> > #define EXT4_ENCRYPT_FL 0x0800 /* encrypted file */
> >
> > f2fs::i_advice
> > #define FADVISE_ENCRYPT_BIT 0x04
> >
> > It's very wired that f2fs didn't use well defined FS_ENCRYPT_FL bit
> > position,
> > result in that we leave a hole in on-disk i_flags, and if we want to show
> > the
> > same 'encrypted' flags status in FS_IOC_GETFLAGS, we need to change more
> > codes.
> >
> > Anyway, I just ask why not let generic status goes into i_flags, and private
> > status goes into i_advices?
>
> Ted and others, what would you say about allocating FS_VERITY_FL as one of the
> unused ext4 / "VFS" inode flags like 0x0100, or maybe 0x0400 if the
> compression flags aren't being used anymore?
>
> I do wish that we separated the on-disk flags namespaces from the
> FS_IOC_GETFLAGS/FS_IOC_SETFLAGS namespace though... Then adding the flag to
Use FS_IOC_{GS}ETXATTR instead?. I'ts not tied to an on-disk format.
Cheers,
Dave.
--
Dave Chinner
da...@fromorbit.com
--
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: fix to show encrypt flag in FS_IOC_GETFLAGS
On 04/02, Chao Yu wrote:
> This patch fixes to show encrypt flag in FS_IOC_GETFLAGS like ext4 does.
Actually, we have to show internal flags owned by f2fs, not generic ones.
We may need to define all of them separately?
>
> Signed-off-by: Chao Yu
> ---
> fs/f2fs/file.c | 9 +++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 8068b015ece5..271fadadaa36 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -1584,8 +1584,13 @@ static int f2fs_ioc_getflags(struct file *filp,
> unsigned long arg)
> {
> struct inode *inode = file_inode(filp);
> struct f2fs_inode_info *fi = F2FS_I(inode);
> - unsigned int flags = fi->i_flags &
> - (FS_FL_USER_VISIBLE | FS_PROJINHERIT_FL);
> + unsigned int flags = fi->i_flags;
> +
> + if (file_is_encrypt(inode))
> + flags |= FS_ENCRYPT_FL;
> +
> + flags &= FS_FL_USER_VISIBLE | FS_PROJINHERIT_FL;
> +
> return put_user(flags, (int __user *)arg);
> }
>
> --
> 2.15.0.55.gc2ece9dc4de6
--
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 04/02, Chao Yu wrote:
> On 2018/3/30 23:39, 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.
>
> Of course, IMO, the most direct way is let user pass image size to mkfs.f2fs
> instead of caculated sector size, I think that will make all happy.
No, it can become too large number.
>
> >
> > 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 those f2fs users, who makes image based on non-512B sector device, once
> they
> upgrade mkfs.f2fs, they will encounter this issue, that may make bad
> reputation.
>
> For interface in f2fs private ioctl, sysfs entry, or syscall, we keep the rule
> that once we want to do some change on it, we will not change the old
> interface
> directly, instead, introduce a new one to keep backward compatibility of old
> one. Still, I hope we can keep that rule in mkfs.f2fs parameter.
Okay, so in order to give more flexible ways, it'd be fine to add
wanted_sector_size by "-w" so that we could blow out all the existing concern
like this.
From: katao
Date: Tue, 27 Mar 2018 13:25:46 +0800
Subject: [PATCH] libf2fs,mkfs.f2fs: add wanted_sector_size for
wanted_total_sectors
The wanted_total_sectors was determined by device sector size, but sometimes
we don't know precise sector_size by default. So, let's give wanted_sector_size
in such the ambiguous situation.
Signed-off-by: katao
Signed-off-by: Junling Zheng
Signed-off-by: Jaegeuk Kim
---
include/f2fs_fs.h | 1 +
lib/libf2fs.c | 13 +
man/mkfs.f2fs.8 | 8
mkfs/f2fs_format_main.c | 6 +-
4 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
index 053ccbe..2d75d39 100644
--- a/include/f2fs_fs.h
+++ b/include/f2fs_fs.h
@@ -335,6 +335,7 @@ struct f2fs_configuration {
u_int64_t device_size;
u_int64_t total_sectors;
u_int64_t wanted_total_sectors;
+ u_int64_t wanted_sector_size;
u_int64_t target_sectors;
u_int32_t sectors_per_blk;
u_int32_t blks_per_seg;
diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index ffdbccb..bb7fe2e 100644
--- a/lib/l
Re: [f2fs-dev] [PATCH] f2fs: reserve bits for fs-verity
[+Cc linux-ext4, linux-fsdevel] On Mon, Apr 02, 2018 at 06:07:10PM +0800, Chao Yu wrote: > Hi Eric and Jaegeuk, > > On 2018/3/31 2:34, Eric Biggers wrote: > > 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 > > Verity flag can also be wrapped by FS_FL_USER_MODIFIABLE like for > FS_ENCRYPT_FL? > > > 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. > > IMO, because this is a VFS feature, it will be better that we can put it in > more > generic place, also user can check this bit in generic way (via > FS_IOC_GETFLAGS), and then for other filesystem who wants add this feature, > that > will be simple to place this bit. > > What I can see is, for encryption feature: > > vfs::i_flags > #define FS_ENCRYPT_FL 0x0800 /* Encrypted file */ > > ext4:i_flags > #define EXT4_ENCRYPT_FL 0x0800 /* encrypted file */ > > f2fs::i_advice > #define FADVISE_ENCRYPT_BIT 0x04 > > It's very wired that f2fs didn't use well defined FS_ENCRYPT_FL bit position, > result in that we leave a hole in on-disk i_flags, and if we want to show the > same 'encrypted' flags status in FS_IOC_GETFLAGS, we need to change more > codes. > > Anyway, I just ask why not let generic status goes into i_flags, and private > status goes into i_advices? Ted and others, what would you say about allocating FS_VERITY_FL as one of the unused ext4 / "VFS" inode flags like 0x0100, or maybe 0x0400 if the compression flags aren't being used anymore? I do wish that we separated the on-disk flags namespaces from the FS_IOC_GETFLAGS/FS_IOC_SETFLAGS namespace though... Then adding the flag to each namespace could be done separately which would make more sense. As-is, the flags are all being conflated, so by allocating a flag in f2fs ->i_flags we would effectively also be allocating it for ext4 and for the UAPI, which we don't necessarily need to do yet. > > > > > 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? > > Since I don't k
Re: [f2fs-dev] [PATCH] mkfs.f2fs: support multiple features with one "-O"
On 2018/4/2 12:19, Junling Zheng wrote:
> Now one "-O" option can support multiple features separated
> by a comma or blank, such as:
> feature1,feature2,... or "feature1 feature2 ..."
At a glance, can last sector number be parsed as feature name?
Thanks,
>
> Signed-off-by: Junling Zheng
> ---
> mkfs/f2fs_format_main.c | 30 +++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/mkfs/f2fs_format_main.c b/mkfs/f2fs_format_main.c
> index 741600e..76ff296 100644
> --- a/mkfs/f2fs_format_main.c
> +++ b/mkfs/f2fs_format_main.c
> @@ -80,10 +80,8 @@ static void f2fs_show_info()
> MSG(0, "Info: Trim is %s\n", c.trim ? "enabled": "disabled");
> }
>
> -static void parse_feature(const char *features)
> +static void set_feature_bits(char *features)
> {
> - while (*features == ' ')
> - features++;
> if (!strcmp(features, "encrypt")) {
> c.feature |= cpu_to_le32(F2FS_FEATURE_ENCRYPT);
> } else if (!strcmp(features, "verity")) {
> @@ -108,6 +106,32 @@ static void parse_feature(const char *features)
> }
> }
>
> +static void parse_feature(const char *features)
> +{
> + char *buf, *sub, *next;
> +
> + buf = calloc(strlen(features) + 1, sizeof(char));
> + ASSERT(buf);
> + strncpy(buf, features, strlen(features) + 1);
> +
> + for (sub = buf; sub && *sub; sub = next ? next + 1 : NULL) {
> + /* Skip the beginning blanks */
> + while (*sub && *sub == ' ')
> + sub++;
> + next = sub;
> + /* Skip a feature word */
> + while (*next && *next != ' ' && *next != ',')
> + next++;
> +
> + if (*next == 0)
> + next = NULL;
> + else
> + *next = 0;
> +
> + set_feature_bits(sub);
> + }
> +}
> +
> static void f2fs_parse_options(int argc, char *argv[])
> {
> static const char *option_string = "qa:c:d:e:E:il:mo:O:s:S:z:t:f";
>
--
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: make assignment of t->dentry_bitmap more readable
In make_dentry_ptr_block, it is confused with "&" for t->dentry_bitmap but without "&" for t->dentry, so delete "&" to make code more readable. Signed-off-by: Yunlong Song --- fs/f2fs/f2fs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 191ee57..474b9e9 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -471,7 +471,7 @@ static inline void make_dentry_ptr_block(struct inode *inode, d->inode = inode; d->max = NR_DENTRY_IN_BLOCK; d->nr_bitmap = SIZE_OF_DENTRY_BITMAP; - d->bitmap = &t->dentry_bitmap; + d->bitmap = t->dentry_bitmap; d->dentry = t->dentry; d->filename = t->filename; } -- 1.8.5.2 -- 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: fix a wrong condition in f2fs_skip_inode_update
On 2018/3/29 19:27, Junling Zheng wrote: > Fix commit 97dd26ad8347 (f2fs: fix wrong AUTO_RECOVER condition). > We should use ~PAGE_MASK to determine whether i_size is aligned to > the f2fs's block size or not. > > Signed-off-by: Junling Zheng Reviewed-by: Chao Yu Thanks, -- 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: fix to show encrypt flag in FS_IOC_GETFLAGS
This patch fixes to show encrypt flag in FS_IOC_GETFLAGS like ext4 does.
Signed-off-by: Chao Yu
---
fs/f2fs/file.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 8068b015ece5..271fadadaa36 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1584,8 +1584,13 @@ static int f2fs_ioc_getflags(struct file *filp, unsigned
long arg)
{
struct inode *inode = file_inode(filp);
struct f2fs_inode_info *fi = F2FS_I(inode);
- unsigned int flags = fi->i_flags &
- (FS_FL_USER_VISIBLE | FS_PROJINHERIT_FL);
+ unsigned int flags = fi->i_flags;
+
+ if (file_is_encrypt(inode))
+ flags |= FS_ENCRYPT_FL;
+
+ flags &= FS_FL_USER_VISIBLE | FS_PROJINHERIT_FL;
+
return put_user(flags, (int __user *)arg);
}
--
2.15.0.55.gc2ece9dc4de6
--
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 and Jaegeuk, On 2018/3/31 2:34, Eric Biggers wrote: > 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 Verity flag can also be wrapped by FS_FL_USER_MODIFIABLE like for FS_ENCRYPT_FL? > 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. IMO, because this is a VFS feature, it will be better that we can put it in more generic place, also user can check this bit in generic way (via FS_IOC_GETFLAGS), and then for other filesystem who wants add this feature, that will be simple to place this bit. What I can see is, for encryption feature: vfs::i_flags #define FS_ENCRYPT_FL 0x0800 /* Encrypted file */ ext4:i_flags #define EXT4_ENCRYPT_FL 0x0800 /* encrypted file */ f2fs::i_advice #define FADVISE_ENCRYPT_BIT 0x04 It's very wired that f2fs didn't use well defined FS_ENCRYPT_FL bit position, result in that we leave a hole in on-disk i_flags, and if we want to show the same 'encrypted' flags status in FS_IOC_GETFLAGS, we need to change more codes. Anyway, I just ask why not let generic status goes into i_flags, and private status goes into i_advices? > > 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? Since I don't know current progress of this feature development, I hope this feature will not be against by vfs developers or suffer design change after we reserved bit for it. :) >> > > 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. Oh, so we want a stable on-disk layout, so image for experience will contain fsverity bit in stable position, after formal android released, image with fsverity feature can s
Re: [f2fs-dev] [PATCH] libf2fs: reset wanted_total_sectors by new sector_size
On 2018/3/30 23:39, 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.
Of course, IMO, the most direct way is let user pass image size to mkfs.f2fs
instead of caculated sector size, I think that will make all happy.
>
> 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 those f2fs users, who makes image based on non-512B sector device, once they
upgrade mkfs.f2fs, they will encounter this issue, that may make bad reputation.
For interface in f2fs private ioctl, sysfs entry, or syscall, we keep the rule
that once we want to do some change on it, we will not change the old interface
directly, instead, introduce a new one to keep backward compatibility of old
one. Still, I hope we can keep that rule in mkfs.f2fs parameter.
>
> 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.
But, for people who upgrade mkfs but not the newly introduced testcase will
encounter this issue.
I just make xfstests as an example, I doubt there will be more user/application
on phone/server/pc scenario may be impacted by this.
Thanks,
>
> 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 *
>> +
