From: Eric Biggers <ebiggers@kernel.org> To: Daeho Jeong <daeho43@gmail.com> Cc: linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, kernel-team@android.com, Daeho Jeong <daehojeong@google.com> Subject: Re: [f2fs-dev] [PATCH] f2fs: change fiemap way in printing compression chunk Date: Wed, 21 Jul 2021 18:15:29 -0700 [thread overview] Message-ID: <YPjGsSEdsoSsCJlB@sol.localdomain> (raw) In-Reply-To: <CACOAw_xeTSa8J_9=+6thXvFT75u734D5asNRogUxt+DC-tPhxg@mail.gmail.com> On Wed, Jul 21, 2021 at 06:04:22PM -0700, Daeho Jeong wrote: > > > > How f2fs stores the mapping information doesn't matter. That's an > > implementation detail that shouldn't be exposed to userspace. The only thing > > that should be exposed is the actual mapping, and for that it seems natural to > > report the physical blocks first. > > > > There is no perfect solution for how to handle the remaining logical blocks, > > given that the fiemap API was not designed for compressed files, but I think we > > should just go with extending the length of the last compressed extent in the > > cluster to cover the remaining logical blocks, i.e.: > > > > [0..31]: 2683128..2683159 flag(0x1009) -> merged, encoded, last_extent > > > > That's what btrfs does on compressed files. > > > > - Eric > > I also agree that that's an implementation detail that shouldn't be > exposed to userspace. > > I want to make it more clear for better appearance. > > Do you think we have to remove "unwritten" information below? I also > think it might be unnecessary information for the user. > [0..31]: 2683128..2683159 flag(0x1009) -> merged, encoded, last_extent > (unwritten?) FIEMAP_EXTENT_UNWRITTEN already has a specific meaning; see Documentation/filesystems/fiemap.rst. It means that the data is all zeroes, and the disk space is preallocated but the data hasn't been written to disk yet. In this case, the data is *not* necessarily all zeroes. So I think FIEMAP_EXTENT_UNWRITTEN shouldn't be used here. > Do you want f2fs to print out the info on a cluster basis, even when > the user asks for one block information? > Like > If the user asks for the info of [8..15], f2fs will return the info of [0..31]? Yes, since that's how FS_IOC_FIEMAP is supposed to work; see Documentation/filesystems/fiemap.rst: All offsets and lengths are in bytes and mirror those on disk. It is valid for an extents logical offset to start before the request or its logical length to extend past the request. (That being said, the f2fs compression+encryption tests I've written don't exercise this case; they only map the whole file at once.) - Eric
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers@kernel.org> To: Daeho Jeong <daeho43@gmail.com> Cc: Daeho Jeong <daehojeong@google.com>, kernel-team@android.com, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH] f2fs: change fiemap way in printing compression chunk Date: Wed, 21 Jul 2021 18:15:29 -0700 [thread overview] Message-ID: <YPjGsSEdsoSsCJlB@sol.localdomain> (raw) In-Reply-To: <CACOAw_xeTSa8J_9=+6thXvFT75u734D5asNRogUxt+DC-tPhxg@mail.gmail.com> On Wed, Jul 21, 2021 at 06:04:22PM -0700, Daeho Jeong wrote: > > > > How f2fs stores the mapping information doesn't matter. That's an > > implementation detail that shouldn't be exposed to userspace. The only thing > > that should be exposed is the actual mapping, and for that it seems natural to > > report the physical blocks first. > > > > There is no perfect solution for how to handle the remaining logical blocks, > > given that the fiemap API was not designed for compressed files, but I think we > > should just go with extending the length of the last compressed extent in the > > cluster to cover the remaining logical blocks, i.e.: > > > > [0..31]: 2683128..2683159 flag(0x1009) -> merged, encoded, last_extent > > > > That's what btrfs does on compressed files. > > > > - Eric > > I also agree that that's an implementation detail that shouldn't be > exposed to userspace. > > I want to make it more clear for better appearance. > > Do you think we have to remove "unwritten" information below? I also > think it might be unnecessary information for the user. > [0..31]: 2683128..2683159 flag(0x1009) -> merged, encoded, last_extent > (unwritten?) FIEMAP_EXTENT_UNWRITTEN already has a specific meaning; see Documentation/filesystems/fiemap.rst. It means that the data is all zeroes, and the disk space is preallocated but the data hasn't been written to disk yet. In this case, the data is *not* necessarily all zeroes. So I think FIEMAP_EXTENT_UNWRITTEN shouldn't be used here. > Do you want f2fs to print out the info on a cluster basis, even when > the user asks for one block information? > Like > If the user asks for the info of [8..15], f2fs will return the info of [0..31]? Yes, since that's how FS_IOC_FIEMAP is supposed to work; see Documentation/filesystems/fiemap.rst: All offsets and lengths are in bytes and mirror those on disk. It is valid for an extents logical offset to start before the request or its logical length to extend past the request. (That being said, the f2fs compression+encryption tests I've written don't exercise this case; they only map the whole file at once.) - Eric _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2021-07-22 1:15 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-21 7:20 [PATCH] f2fs: change fiemap way in printing compression chunk Daeho Jeong 2021-07-21 7:20 ` [f2fs-dev] " Daeho Jeong 2021-07-21 21:35 ` Eric Biggers 2021-07-21 21:35 ` Eric Biggers 2021-07-21 22:30 ` Daeho Jeong 2021-07-21 22:30 ` Daeho Jeong 2021-07-22 0:15 ` Eric Biggers 2021-07-22 0:15 ` Eric Biggers 2021-07-22 1:04 ` Daeho Jeong 2021-07-22 1:04 ` Daeho Jeong 2021-07-22 1:15 ` Eric Biggers [this message] 2021-07-22 1:15 ` Eric Biggers 2021-07-22 1:40 ` Daeho Jeong 2021-07-22 1:40 ` Daeho Jeong 2021-07-22 1:56 ` Eric Biggers 2021-07-22 1:56 ` Eric Biggers 2021-07-22 3:59 ` Daeho Jeong 2021-07-22 3:59 ` Daeho Jeong [not found] ` <CACOAw_yfG494AK=XH_xzeTDWn-a1mYF+537=VTT6oX6RgLGxnw@mail.gmail.com> 2021-07-22 14:34 ` Eric Biggers 2021-07-22 16:10 ` Daeho Jeong 2021-07-22 16:17 ` Eric Biggers 2021-07-22 16:26 ` Eric Biggers 2021-07-22 16:33 ` Daeho Jeong 2021-07-22 17:36 ` Eric Biggers 2021-07-22 17:55 ` Daeho Jeong 2021-07-22 17:57 ` Eric Biggers 2021-07-22 6:24 ` Christoph Hellwig 2021-07-22 6:24 ` [f2fs-dev] " Christoph Hellwig 2021-07-22 6:39 ` Daeho Jeong 2021-07-22 6:39 ` [f2fs-dev] " Daeho Jeong
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YPjGsSEdsoSsCJlB@sol.localdomain \ --to=ebiggers@kernel.org \ --cc=daeho43@gmail.com \ --cc=daehojeong@google.com \ --cc=kernel-team@android.com \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.