fat: Batched discard support for fat

fs/fat/fatent.c +8 -0

+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	struct fatent_operations *ops = sbi->fatent_ops;
+	struct fat_entry fatent;
+	unsigned long reada_blocks, reada_mask, cur_block;
+	int err, free, count, entry;
+	int start, len, minlen, trimmed;

fs/fat/fatent.c +4 -0

+	if (start >= sbi->max_cluster)
+		goto out;
+
+	len = (len > sbi->max_cluster) ? sbi->max_cluster : len;

Quoted 8 lines · Kyungmin Park

fs/fat/fatent.c +8 -0

+int fat_trim_fs(struct super_block *sb, struct fstrim_range *range)
+{
+     struct msdos_sb_info *sbi = MSDOS_SB(sb);
+     struct fatent_operations *ops = sbi->fatent_ops;
+     struct fat_entry fatent;
+     unsigned long reada_blocks, reada_mask, cur_block;
+     int err, free, count, entry;
+     int start, len, minlen, trimmed;

fs/fat/fatent.c out: · line 712

Quoted 10 lines

fs/fat/fatent.c +10 -0

+
+     start = range->start >> sb->s_blocksize_bits;
+     len = range->len >> sb->s_blocksize_bits;
+     len = round_down(start + len, sbi->sec_per_clus);
+     start = round_up(start, sbi->sec_per_clus);
+     minlen = range->minlen >> sb->s_blocksize_bits;
+     minlen = round_up(minlen, sbi->sec_per_clus);
+     trimmed = 0;
+     count = 0;
+     err = -EINVAL;

Sorry for didn't mention at previous. You can use ->cluster_size, and ->cluster_bits.

Quoted 4 lines

+     if (start >= sbi->max_cluster)
+             goto out;
+
+     len = (len > sbi->max_cluster) ? sbi->max_cluster : len;

[...]

Quoted 7 lines

+             trimmed += free;
+     }
+     range->len = (u64)(trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
+     fatent_brelse(&fatent);
+out:
+     unlock_fat(sbi);
+     return err;

Again, this ioctl's design is unclear, and seems to be strange. I wouldn't want to add this before clearing it. Please explain what is right behavior.

E.g. if user specified 0-1024 and FS data block was actually started at 2048. What is right behavior? And if the end of blocks, what returned? For now, it seems to return range->len == 0 on both cases.

Well, so, my suggestion is providing this like flat one extent file. I.e. FS have to map actual block placement to flat. And result also like write/read (return bytes as trimed, and at EOF returns 0).

Thanks.
--
OGAWA Hirofumi <[email protected]>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 3 lines

Again, this ioctl's design is unclear, and seems to be strange. I wouldn't want to add this before clearing it. Please explain what is right behavior.

Umm it's out of my scope. it's trim design.
See also btrfs batched discard support. it's also no consideration as you mentioned.

As I know, now xfs, ext4, and btrfs support this fstrim without these concern.

http://thread.gmane.org/gmane.comp.file-systems.btrfs/9758

Quoted 10 lines · Kyungmin Park

Quoted 3 lines

Again, this ioctl's design is unclear, and seems to be strange. I wouldn't want to add this before clearing it. Please explain what is right behavior.

Umm it's out of my scope. it's trim design.
See also btrfs batched discard support. it's also no consideration as you mentioned.

As I know, now xfs, ext4, and btrfs support this fstrim without these concern.

Quoted 1 line

http://thread.gmane.org/gmane.comp.file-systems.btrfs/9758

I looked at fstrim.c. It is lowlevel tools to just issue FITRIM - it doesn't use the result at all.

Um. Honestly I'd like to see more or wait until real user for now, instead of providing unclear design.

Thanks.
--
OGAWA Hirofumi <[email protected]>

Quoted 5 lines

I looked at fstrim.c. It is lowlevel tools to just issue FITRIM - it doesn't use the result at all.

Um. Honestly I'd like to see more or wait until real user for now, instead of providing unclear design.

You can find the testcase xfs251(?)

and it's our our usage. After UMS support finished, execute the trim command from 0 to MAXINT.
and periodically call the trim FAT partitions.

So you will go through (blocks, bytes...) 0 -> 20

OOOO==O===OO===OOOOO==O===O===OOOOOOO===
^ ^
0 20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

instead of

0-3
10-11
15-19
30-36

Quoted 13 lines · Kyungmin Park

So you will go through (blocks, bytes...) 0 -> 20

OOOO==O===OO===OOOOO==O===O===OOOOOOO===
^ ^
0 20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

Quoted 6 lines

instead of

0-3
10-11
15-19
30-36

Sorry for joining the discussion late, but shouldn't you also pass the alignment of the discards?

FAT is typically used on cheap media that have very limited support for garbage-collection, such as eMMC or SD cards.

On most SDHC cards, you only ever want to issue discard on full erase blocks (allocation units per spec), typically sized 4 MB.

If you just pass the minimum length, the file system could end up erasing a 4 MB section that spans two half erase blocks, or it could span a few clusters of the following erase block, both of which is not desirable from a performance point of view.

On other media, you have the same problem inside an erase block:
These might be able to discard parts of an erase block efficiently, but normally not less than a flash page (typically 8 to 32 KB).

Again, you don't want to discard partial pages in this case, and that is much more important than discarding a large number of pages because it would result in an immediate copy-on-write operation.

Further, when you erase some pages inside of an erase block, you probably should not span multiple erase blocks but instead issue separate requests for each set of pages in one erase block.

Arnd

Arnd

Quoted 21 lines · Arnd Bergmann

Quoted 13 lines · Kyungmin Park

So you will go through (blocks, bytes...) 0 -> 20

  OOOO==O===OO===OOOOO==O===O===OOOOOOO===
  ^                   ^
  0                   20

So, you will call discard on extents:

0-3
You'll skip 6 because is smaller than minlen
10-11
15-19

Quoted 6 lines

instead of

0-3
10-11
15-19
30-36

Quoted 8 lines

Sorry for joining the discussion late, but shouldn't you also pass the alignment of the discards?

FAT is typically used on cheap media that have very limited support for garbage-collection, such as eMMC or SD cards.

On most SDHC cards, you only ever want to issue discard on full erase blocks (allocation units per spec), typically sized 4 MB.

I was not aware of the fact that SD cards (etc..) does have garbage collection of some sort, or that they even have support discard, since I thought that we have only TRIM,UNAMP/WRITE_SAME comands for SATA or SCSI drives.

Or is there some sort of kernel mechanism doing garbage collection such is this for the cheap media ?

Quoted 1 line

Quoted 4 lines

If you just pass the minimum length, the file system could end up erasing a 4 MB section that spans two half erase blocks, or it could span a few clusters of the following erase block, both of which is not desirable from a performance point of view.

Does those cards export such information correctly ?

Quoted 5 lines

On other media, you have the same problem inside an erase block:
These might be able to discard parts of an erase block efficiently, but normally not less than a flash page (typically 8 to 32 KB).

Well I have tested several SSD's and thinly provisioned devices, but I have not seen any strange behaviour, other than it was terribly unefficient to do so. See my results here:

http://people.redhat.com/lczerner/discard/test_discard.html

the fact is that I have not tried discard size smaller than 4K, since this is the most usual block size for the filesystem.

Quoted 8 lines

Again, you don't want to discard partial pages in this case, and that is much more important than discarding a large number of pages because it would result in an immediate copy-on-write operation.

Further, when you erase some pages inside of an erase block, you probably should not span multiple erase blocks but instead issue separate requests for each set of pages in one erase block.

Does it mean that we should not issue bigger discards that erase block ? That does not sound good given my test results. Or maybe I misunderstood your point ?

Quoted 5 lines

      Arnd

      Arnd

Thanks!
-Lukas

Quoted 6 lines

Quoted 4 lines

If you just pass the minimum length, the file system could end up erasing a 4 MB section that spans two half erase blocks, or it could span a few clusters of the following erase block, both of which is not desirable from a performance point of view.

Does those cards export such information correctly ?

Each chip vendor knows it exactly and usually can't tell it outside officially.

but it's not big as you think.

also if the request size is under trim size, then it's discarded internally. I mean do nothing at firmware level.

Quoted 8 lines · Arnd Bergmann

Sorry for joining the discussion late, but shouldn't you also pass the alignment of the discards?

FAT is typically used on cheap media that have very limited support for garbage-collection, such as eMMC or SD cards.

On most SDHC cards, you only ever want to issue discard on full erase blocks (allocation units per spec), typically sized 4 MB.

I was not aware of the fact that SD cards (etc..) does have garbage collection of some sort, or that they even have support discard, since I thought that we have only TRIM,UNAMP/WRITE_SAME comands for SATA or SCSI drives.

Or is there some sort of kernel mechanism doing garbage collection such is this for the cheap media ?

Quoted 4 lines

If you just pass the minimum length, the file system could end up erasing a 4 MB section that spans two half erase blocks, or it could span a few clusters of the following erase block, both of which is not desirable from a performance point of view.

Does those cards export such information correctly ?

Quoted 3 lines

On other media, you have the same problem inside an erase block:
These might be able to discard parts of an erase block efficiently, but normally not less than a flash page (typically 8 to 32 KB).

Well I have tested several SSD's and thinly provisioned devices, but I have not seen any strange behaviour, other than it was terribly unefficient to do so. See my results here:

http://people.redhat.com/lczerner/discard/test_discard.html

the fact is that I have not tried discard size smaller than 4K, since this is the most usual block size for the filesystem.

Quoted 7 lines

Again, you don't want to discard partial pages in this case, and that is much more important than discarding a large number of pages because it would result in an immediate copy-on-write operation.

Further, when you erase some pages inside of an erase block, you probably should not span multiple erase blocks but instead issue separate requests for each set of pages in one erase block.

Does it mean that we should not issue bigger discards that erase block ? That does not sound good given my test results. Or maybe I misunderstood your point ?

Quoted 10 lines · Lukas Czerner

Quoted 8 lines · Arnd Bergmann

Sorry for joining the discussion late, but shouldn't you also pass the alignment of the discards?

FAT is typically used on cheap media that have very limited support for garbage-collection, such as eMMC or SD cards.

On most SDHC cards, you only ever want to issue discard on full erase blocks (allocation units per spec), typically sized 4 MB.

Quoted 7 lines

I was not aware of the fact that SD cards (etc..) does have garbage collection of some sort, or that they even have support discard, since I thought that we have only TRIM,UNAMP/WRITE_SAME comands for SATA or SCSI drives.

Or is there some sort of kernel mechanism doing garbage collection such is this for the cheap media ?

The garbage collection is what happens on the device internally. Each card has only a small number of erase blocks that it can write to at a time (between 1 and ten, typically). When you "open" a new erase block by writing to it, the card will "close" another erase block by garbage-collecting the data, i.e. it copies the data that has been recently written together with the data that was in the erase block previously and has not been touched since the last GC into a single erase block, and then erases ones that has been freed up.

I've explained this in more detail a few weeks ago in an article, see
https://lwn.net/Articles/428584/.

Discarding full erase blocks makes the wear levelling more efficient, since there is a larger number of unused erase blocks to choose from.

The SD card standard does not specify what happens when you erase less than an erase block, other than that the data will be in a known state (all-ones or all-zeros). On cheap cards that might require actually writing that data, while better ones use a sector remapping inside of an erase block that makes is possible mark sectors or pages as unused, which can speed up future garbage collection on the erase block.

Quoted 6 lines

Quoted 4 lines

If you just pass the minimum length, the file system could end up erasing a 4 MB section that spans two half erase blocks, or it could span a few clusters of the following erase block, both of which is not desirable from a performance point of view.

Does those cards export such information correctly ?

Most of the time, they export the erase block size correctly, but sometimes they lie. In fact, all of the cards I've seen report 4 MB erase blocks, but some cards in fact use 1.5 MB, 2 MB or 8 MB instead. I'm sure we will see 3 MB, 6 MB, 12 MB and 16 MB soon.

SD cards do not export the page size, but most of them today use 16 KB. See my list at

https://wiki.linaro.org/WorkingGroups/KernelConsolidation/Projects/FlashCardSurvey

Quoted 9 lines

Quoted 4 lines

On other media, you have the same problem inside an erase block:
These might be able to discard parts of an erase block efficiently, but normally not less than a flash page (typically 8 to 32 KB).

Well I have tested several SSD's and thinly provisioned devices, but I have not seen any strange behaviour, other than it was terribly unefficient to do so. See my results here:

Quoted 4 lines

http://people.redhat.com/lczerner/discard/test_discard.html

the fact is that I have not tried discard size smaller than 4K, since this is the most usual block size for the filesystem.

Good SSDs can deal with remapping 4k blocks, while cheap ones (SD cards, USB sticks, or low-end ATA SSDs) can only do operations on full blocks.

Quoted 5 lines

Quoted 5 lines

Again, you don't want to discard partial pages in this case, and that is much more important than discarding a large number of pages because it would result in an immediate copy-on-write operation.

Quoted 7 lines

Quoted 3 lines

Further, when you erase some pages inside of an erase block, you probably should not span multiple erase blocks but instead issue separate requests for each set of pages in one erase block.

Does it mean that we should not issue bigger discards that erase block ? That does not sound good given my test results. Or maybe I misunderstood your point ?

I think ideally you want to have multiples of full erase blocks, but I would not combine discards of a partial erase block with any other discard.

       Arnd

Quoted 15 lines · Arnd Bergmann

Quoted 6 lines

Quoted 4 lines

If you just pass the minimum length, the file system could end up erasing a 4 MB section that spans two half erase blocks, or it could span a few clusters of the following erase block, both of which is not desirable from a performance point of view.

Does those cards export such information correctly ?

Most of the time, they export the erase block size correctly, but sometimes they lie. In fact, all of the cards I've seen report 4 MB erase blocks, but some cards in fact use 1.5 MB, 2 MB or 8 MB instead. I'm sure we will see 3 MB, 6 MB, 12 MB and 16 MB soon.

SD cards do not export the page size, but most of them today use 16 KB. See my list at

Quoted 1 line

https://wiki.linaro.org/WorkingGroups/KernelConsolidation/Projects/FlashCardSurvey

Thank you for your effort. but think it the original purpose of batched discard feature.
Do you want to run the batched discard for SD card. I mean usually in our environment SD card is optional.
and my goal is used for eMMC. and you know, eMMC and SD card internal algorithm is different.

The goal of SD card is performance but eMMC is different. it should consider the reliability also. e.g., Sudden Power Off Recovery.

Quoted 3 lines

Good SSDs can deal with remapping 4k blocks, while cheap ones (SD cards, USB sticks, or low-end ATA SSDs) can only do operations on full blocks.

It really depends on devices. To increase the performance we have to increase the channel and the way unit at internally.
Most NAND devices are similar there's no way except increase the interleaving unit. and it affects the price.

That's reason I like the batched discard. it's not easy to guess the erase block at system level. so it's helpful scan the unused blocks and try to trim at once.

Please remember that the discard is just hint. If we write the flash sequentially then we maybe don't need to trim it.

Quoted 16 lines · Kyungmin Park

Quoted 15 lines · Arnd Bergmann

Quoted 6 lines

Quoted 4 lines

If you just pass the minimum length, the file system could end up erasing a 4 MB section that spans two half erase blocks, or it could span a few clusters of the following erase block, both of which is not desirable from a performance point of view.

Does those cards export such information correctly ?

Most of the time, they export the erase block size correctly, but sometimes they lie. In fact, all of the cards I've seen report 4 MB erase blocks, but some cards in fact use 1.5 MB, 2 MB or 8 MB instead. I'm sure we will see 3 MB, 6 MB, 12 MB and 16 MB soon.

SD cards do not export the page size, but most of them today use 16 KB. See my list at

Quoted 8 lines

Quoted 1 line

https://wiki.linaro.org/WorkingGroups/KernelConsolidation/Projects/FlashCardSurvey

Thank you for your effort. but think it the original purpose of batched discard feature.
Do you want to run the batched discard for SD card. I mean usually in our environment SD card is optional.
and my goal is used for eMMC. and you know, eMMC and SD card internal algorithm is different.

I've done only a few measurements on eMMC, but for all I know, they use the same fundamental principles, just with a slightly different command set and physical interface. A number of manufacturers advertise controller chips that can be integrated on either MMC or SD packages, presumably using a different firmware.

Quoted 2 lines

The goal of SD card is performance but eMMC is different. it should consider the reliability also. e.g., Sudden Power Off Recovery.

I would have guessed the opposite, that sudden power off is much more important on SD cards that can simply be removed while writing.

In practice, I would expect very little effort getting put into reliability on either of the two.

Note that there was a patch on the mmc mailing list very recently to support the reliable write mode of eMMC.

Also, when you want reliablility, you probably shouldn't use the FAT file system, since it lacks transactional writes. If you target eMMC media, there is usually no advantage in using FAT because the data does not get accessed by other operating systems.

Quoted 11 lines

Quoted 3 lines

Good SSDs can deal with remapping 4k blocks, while cheap ones (SD cards, USB sticks, or low-end ATA SSDs) can only do operations on full blocks.

It really depends on devices. To increase the performance we have to increase the channel and the way unit at internally.
Most NAND devices are similar there's no way except increase the interleaving unit. and it affects the price.

That's reason I like the batched discard. it's not easy to guess the erase block at system level. so it's helpful scan the unused blocks and try to trim at once.

I'm absolutely in favor of batched discard, I just think we should apply some stricter rules to what blocks can get batched.

Quoted 2 lines

Please remember that the discard is just hint. If we write the flash sequentially then we maybe don't need to trim it.

With the current trends in NAND technology, the number of erase cycles keep going down, so I'd assume that we increasingly rely on discard in order to keep the wear levelling working, at least for the media that only do dynamic wear levelling. When you have a drive that supports static wear levelling, discard becomes more a performance optimization than a reliability requirement.

       Arnd

Hi again,

Do you still object to merge this feature for .40? As I said the batched discard design is out-of-scope of this patch. It's implemented at other batched discard, ext4, xfs and so on.

I hope fat is also support the batched discard.

Any opinions?

Quoted 1 line · Kyungmin Park

Hi again,

Hi

Quoted 7 lines

Do you still object to merge this feature for .40? As I said the batched discard design is out-of-scope of this patch. It's implemented at other batched discard, ext4, xfs and so on.

I hope fat is also support the batched discard.

Any opinions?

I'm also thinking implementing this is good though. Sorry, I'm not going to apply this for now, and would like to wait to be used by real userland (I hope guys notice the problem, or userland tackles it somehow sadly).

I think, to expose the wrong behavior like this would be worse.

E.g. one of problems, userland might do like this (trim chunk from 0 to number of block)

for chunk in number_of_blocks
       do_trim chunk
done

But this is actually wrong, this interface doesn't map blocks to 0-max, so userland have to know real maximum-block-number somehow for each FS (and maybe have to know real minimum-block-number).

So, how to fix this? The solutions would be userland workaround, or fix kernel. If it was userland, userland have to know FS internal sadly. If it was kernel, we would have backward compatible nightmare, or ignore compatible :(.

I really think we have to rethink this and have agreement about common interface. Or there was real userland app, I think FAT can implement to work that app.

Quoted 13 lines

Quoted 7 lines

Do you still object to merge this feature for .40? As I said the batched discard design is out-of-scope of this patch. It's implemented at other batched discard, ext4, xfs and so on.

I hope fat is also support the batched discard.

Any opinions?

I'm also thinking implementing this is good though. Sorry, I'm not going to apply this for now, and would like to wait to be used by real userland (I hope guys notice the problem, or userland tackles it somehow sadly).

Quoted 8 lines

I think, to expose the wrong behavior like this would be worse.

E.g. one of problems, userland might do like this (trim chunk from 0 to number of block)

for chunk in number_of_blocks
       do_trim chunk
done

It's handled at trim implementation. It just trim the fat aware block. Not trim the blocks which fat doesn't know.
As fat don't use the block 0, 1, it adjust the start block at kernel.

(quoted diff) +2 -0

+       if (start < FAT_START_ENT)
+               start = FAT_START_ENT;

and don't exceed the max cluster size.

Quoted 8 lines

But this is actually wrong, this interface doesn't map blocks to 0-max, so userland have to know real maximum-block-number somehow for each FS (and maybe have to know real minimum-block-number).

So, how to fix this? The solutions would be userland workaround, or fix kernel. If it was userland, userland have to know FS internal sadly. If it was kernel, we would have backward compatible nightmare, or ignore compatible :(.

I think basic concept of batched discard is trim at once instead of deleted entries at filesystem (original one).
So it can set the specific start address or any start (usually 0) with maximum length.

Quoted 3 lines

I really think we have to rethink this and have agreement about common interface. Or there was real userland app, I think FAT can implement to work that app.

IMO, we should use the same user space application. user program doesn't know the which filesystem are underlying.

So sample user space program used at ext4, xfs should be used. and I tested it.

Quoted 33 lines · Kyungmin Park

Quoted 13 lines

Quoted 7 lines

Do you still object to merge this feature for .40? As I said the batched discard design is out-of-scope of this patch. It's implemented at other batched discard, ext4, xfs and so on.

I hope fat is also support the batched discard.

Any opinions?

I'm also thinking implementing this is good though. Sorry, I'm not going to apply this for now, and would like to wait to be used by real userland (I hope guys notice the problem, or userland tackles it somehow sadly).

Quoted 8 lines

I think, to expose the wrong behavior like this would be worse.

E.g. one of problems, userland might do like this (trim chunk from 0 to number of block)

for chunk in number_of_blocks
       do_trim chunk
done

It's handled at trim implementation. It just trim the fat aware block. Not trim the blocks which fat doesn't know.
As fat don't use the block 0, 1, it adjust the start block at kernel.

+       if (start < FAT_START_ENT)
+               start = FAT_START_ENT;

and don't exceed the max cluster size.

+       len = (len > sbi->max_cluster) ? sbi->max_cluster : len;

Quoted 1 line

+       for (count = start; count <= len; count++) {

Yes. We _adjust_ from 0 to 2 here, so, the end of block also have to be _adjusted_.

From other point of view, if userland specified 0 - max-length (i.e. number of blocks), what happens? It would trim block of 2 - (max-length - 2), right?

So, actually, userland specify 2 - (max-length + 2). And userland has to know 2 is real start, not 0.

But how to know it? Yes, it's FS internal layout, AFAICS, other FSes also expose internal like this.

Quoted 10 lines

Quoted 9 lines

But this is actually wrong, this interface doesn't map blocks to 0-max, so userland have to know real maximum-block-number somehow for each FS (and maybe have to know real minimum-block-number).

So, how to fix this? The solutions would be userland workaround, or fix kernel. If it was userland, userland have to know FS internal sadly. If it was kernel, we would have backward compatible nightmare, or ignore compatible :(.

Quoted 4 lines

I think basic concept of batched discard is trim at once instead of deleted entries at filesystem (original one).
So it can set the specific start address or any start (usually 0) with maximum length.

Yes, I think so too (i.e. 0 - max length). But the implement is not working like it. It exposes FS internal layout.

Quoted 6 lines

Quoted 3 lines

I really think we have to rethink this and have agreement about common interface. Or there was real userland app, I think FAT can implement to work that app.

IMO, we should use the same user space application. user program doesn't know the which filesystem are underlying.

Indeed.

Quoted 2 lines

So sample user space program used at ext4, xfs should be used. and I tested it.

I bet it doesn't trim whole FS (due to expose FS layout) actually even if user asked like above example.

Thanks.
--
OGAWA Hirofumi <[email protected]>

Quoted 13 lines

Quoted 12 lines

It's handled at trim implementation. It just trim the fat aware block. Not trim the blocks which fat doesn't know.
As fat don't use the block 0, 1, it adjust the start block at kernel.

+       if (start < FAT_START_ENT)
+               start = FAT_START_ENT;

and don't exceed the max cluster size.

+       len = (len > sbi->max_cluster) ? sbi->max_cluster : len;

+       for (count = start; count <= len; count++) {

Quoted 6 lines

Yes. We _adjust_ from 0 to 2 here, so, the end of block also have to be _adjusted_.

From other point of view, if userland specified 0 - max-length (i.e. number of blocks), what happens? It would trim block of 2 - (max-length - 2), right?

No, length is not changed. so max-length is used.

Quoted 13 lines · Kyungmin Park

Quoted 13 lines

Quoted 12 lines

It's handled at trim implementation. It just trim the fat aware block. Not trim the blocks which fat doesn't know.
As fat don't use the block 0, 1, it adjust the start block at kernel.

+       if (start < FAT_START_ENT)
+               start = FAT_START_ENT;

and don't exceed the max cluster size.

+       len = (len > sbi->max_cluster) ? sbi->max_cluster : len;

+       for (count = start; count <= len; count++) {

Quoted 8 lines

Quoted 6 lines

Yes. We _adjust_ from 0 to 2 here, so, the end of block also have to be _adjusted_.

From other point of view, if userland specified 0 - max-length (i.e. number of blocks), what happens? It would trim block of 2 - (max-length - 2), right?

No, length is not changed. so max-length is used.

No, no. Userland will know max-length from statvfs, right? So, let's assume it is 100 (->f_blocks) * 1024 (->f_bsize).

Now, userland know about max length, 102400, ok? Let's start to trim.

Assume, userland want to trim whole. So, userland will specify like

       trim(0, 102400).

What happen in kernel actually?

Current implement doesn't map blocks. So, in the case of FAT, it adjusts from 0 to 2 * 1024.

So, it trims between 2048 and 102400. The problem is here. FS layout is actually, 2048 and (102400 + 2048). I.e. actually userland has to do

       trim(2048, 102400 + 2048)

to specify whole. How to know 2048?

See what I'm saying?

FAT has liner block space, so the problem is small against mapping. But other FSes has bigger problem.

Thanks.
--
OGAWA Hirofumi <[email protected]>

Quoted 16 lines

Quoted 1 line

No, length is not changed. so max-length is used.

No, no. Userland will know max-length from statvfs, right? So, let's assume it is 100 (->f_blocks) * 1024 (->f_bsize).

Now, userland know about max length, 102400, ok? Let's start to trim.

Assume, userland want to trim whole. So, userland will specify like

       trim(0, 102400).

What happen in kernel actually?

Current implement doesn't map blocks. So, in the case of FAT, it adjusts from 0 to 2 * 1024.

Quoted 4 lines

So, it trims between 2048 and 102400. The problem is here. FS layout is actually, 2048 and (102400 + 2048). I.e. actually userland has to do

       trim(2048, 102400 + 2048)

Umm maybe first implementation does as like this, but Lukas mentioned it's wrong. So I modified it for batched discard concept.

You want the loop like this

for (count = start; count <= (start + len); count++)

Quoted 16 lines · Kyungmin Park

Quoted 16 lines

Quoted 4 lines

It's handled at trim implementation. It just trim the fat aware block. Not trim the blocks which fat doesn't know.
As fat don't use the block 0, 1, it adjust the start block at kernel.

Quoted 8 lines

+       if (start < FAT_START_ENT)
+               start = FAT_START_ENT;

and don't exceed the max cluster size.

+       len = (len > sbi->max_cluster) ? sbi->max_cluster : len;

+       for (count = start; count <= len; count++) {

Yes. We _adjust_ from 0 to 2 here, so, the end of block also have to be _adjusted_.

Quoted 5 lines

Quoted 3 lines

From other point of view, if userland specified 0 - max-length (i.e. number of blocks), what happens? It would trim block of 2 - (max-length - 2), right?

No, length is not changed. so max-length is used.

No, no. Userland will know max-length from statvfs, right? So, let's assume it is 100 (->f_blocks) * 1024 (->f_bsize).

Now, userland know about max length, 102400, ok? Let's start to trim.

Assume, userland want to trim whole. So, userland will specify like

	trim(0, 102400).

What happen in kernel actually?

Current implement doesn't map blocks. So, in the case of FAT, it adjusts from 0 to 2 * 1024.

So, it trims between 2048 and 102400. The problem is here. FS layout is actually, 2048 and (102400 + 2048). I.e. actually userland has to do

	trim(2048, 102400 + 2048)

to specify whole. How to know 2048?

See what I'm saying?

FAT has liner block space, so the problem is small against mapping. But other FSes has bigger problem.

Thanks.

Quoted 2 lines

No, no. Userland will know max-length from statvfs, right? So, let's assume it is 100 (->f_blocks) * 1024 (->f_bsize).

You do not need to know the filesystem size to do the discard, it should be adjusted within the kernel. Just specify ULLONG_MAX as a length. See fstrim tool in util-linux-ng.

Quoted 12 lines

Now, userland know about max length, 102400, ok? Let's start to trim.

Assume, userland want to trim whole. So, userland will specify like

	trim(0, 102400).

What happen in kernel actually?

Current implement doesn't map blocks. So, in the case of FAT, it adjusts from 0 to 2 * 1024.

Quoted 6 lines

So, it trims between 2048 and 102400. The problem is here. FS layout is actually, 2048 and (102400 + 2048). I.e. actually userland has to do

	trim(2048, 102400 + 2048)

to specify whole. How to know 2048?

You do not need to know anything in userspace. If you want to trim the whole filesystem you just do trim(0, ULLONG_MAX) - which is what fstrim does when you do not specify range. And you just skip the filesystem metadata obviously, regardless if they are at the beginning of the filesystem or in the middle. Just do whatever you need to do within your filesystem.

What we do in ext4 is, that we convert length and start passed in struct fstrim_range into filesystem block units and then get the last allocation group and block offset within that group (we do the same for the start block) and we try to discard free block ranges in from staring block to the last block.

It is really not a rocket science and since every filesystem is different and has different internal data structures it is up to you how to do this. And if you shift a block or two, it really does not matter as much since user-land does not know about how the filesystem block are laid out anyway, nor user land knows which are free and which are not.

I agree that the interface is a little bit fuzzy, but that is mainly because it is intended to be filesystem independent and we do have a lot of various filesystems, so I wanted it to be as flexibile as it should, hence the start, len in Bytes.

Hope it helped.

Quoted 50 lines · Lukas Czerner

Quoted 2 lines

No, no. Userland will know max-length from statvfs, right? So, let's assume it is 100 (->f_blocks) * 1024 (->f_bsize).

You do not need to know the filesystem size to do the discard, it should be adjusted within the kernel. Just specify ULLONG_MAX as a length. See fstrim tool in util-linux-ng.

Quoted 12 lines

Now, userland know about max length, 102400, ok? Let's start to trim.

Assume, userland want to trim whole. So, userland will specify like

	trim(0, 102400).

What happen in kernel actually?

Current implement doesn't map blocks. So, in the case of FAT, it adjusts from 0 to 2 * 1024.

Quoted 6 lines

So, it trims between 2048 and 102400. The problem is here. FS layout is actually, 2048 and (102400 + 2048). I.e. actually userland has to do

	trim(2048, 102400 + 2048)

to specify whole. How to know 2048?

You do not need to know anything in userspace. If you want to trim the whole filesystem you just do trim(0, ULLONG_MAX) - which is what fstrim does when you do not specify range. And you just skip the filesystem metadata obviously, regardless if they are at the beginning of the filesystem or in the middle. Just do whatever you need to do within your filesystem.

What we do in ext4 is, that we convert length and start passed in struct fstrim_range into filesystem block units and then get the last allocation group and block offset within that group (we do the same for the start block) and we try to discard free block ranges in from staring block to the last block.

It is really not a rocket science and since every filesystem is different and has different internal data structures it is up to you how to do this. And if you shift a block or two, it really does not matter as much since user-land does not know about how the filesystem block are laid out anyway, nor user land knows which are free and which are not.

I agree that the interface is a little bit fuzzy, but that is mainly because it is intended to be filesystem independent and we do have a lot of various filesystems, so I wanted it to be as flexibile as it should, hence the start, len in Bytes.

Quoted 1 line

Hope it helped.

No. If you want to trim whole with some chunk like 1GB and periodically (IIRC in xfstest), what do? We have to trim until ULLONG_MAX for each 1GB?

Thanks.

Quoted 6 lines

No. If you want to trim whole with some chunk like 1GB and periodically (IIRC in xfstest), what do? We have to trim until ULLONG_MAX for each 1GB?

Thanks.

What ? No, of course not. As I said, just go through 1G worth of filesystem blocks skipping metadata. However we do have a special case when we adjust start and len according to the first data block (which is only the case of 1024B blocksize).

	if (start < first_data_blk) {
		len -= first_data_blk - start;
		start = first_data_blk;
	}

Which means that we just skip the first block (or whatever first data block is). And this is the same as skipping metadata.

Quoted 17 lines · Lukas Czerner

Quoted 6 lines

No. If you want to trim whole with some chunk like 1GB and periodically (IIRC in xfstest), what do? We have to trim until ULLONG_MAX for each 1GB?

Thanks.

What ? No, of course not. As I said, just go through 1G worth of filesystem blocks skipping metadata. However we do have a special case when we adjust start and len according to the first data block (which is only the case of 1024B blocksize).

	if (start < first_data_blk) {
		len -= first_data_blk - start;
		start = first_data_blk;
	}

Quoted 2 lines

Which means that we just skip the first block (or whatever first data block is). And this is the same as skipping metadata.

Are you read my email? So, FAT adjust 2 blocks, ext* 1 block, and what is other? The middle was guaranteed as continued? So, which is end of blocks?

Quoted 3 lines

Are you read my email? So, FAT adjust 2 blocks, ext* 1 block, and what is other? The middle was guaranteed as continued? So, which is end of blocks?

I am sorry but I am not sure what you are asking for. Just grab the size of the file system or even better the size of underlying device (since the filesyste would not be bigger than that) and use that.

Of course if you grab the count of filesystem blocks (and the file system does not account for the first data block) you'll end up with len smaller than first data block. So it might make sense to not to decrement the len and just start from first data block since it is not accounted for anyway. However it is not a big deal, is it ? Are you expecting any problems with this behaviour ?

Quoted 8 lines · Lukas Czerner

Quoted 3 lines

Are you read my email? So, FAT adjust 2 blocks, ext* 1 block, and what is other? The middle was guaranteed as continued? So, which is end of blocks?

I am sorry but I am not sure what you are asking for. Just grab the size of the file system or even better the size of underlying device (since the filesyste would not be bigger than that) and use that.

Quoted 6 lines

Of course if you grab the count of filesystem blocks (and the file system does not account for the first data block) you'll end up with len smaller than first data block. So it might make sense to not to decrement the len and just start from first data block since it is not accounted for anyway. However it is not a big deal, is it ? Are you expecting any problems with this behaviour ?

What is size of file system or underlying devices? You force to find the device which target FS is using? Even if you can get size of underlying devices, you force to user to insane loop in size of devices?

Why can you guarantee it's not big deal in design? Why can't you admit userland can't make optimized loop?

Quoted 3 lines

What is size of file system or underlying devices? You force to find the device which target FS is using? Even if you can get size of underlying devices, you force to user to insane loop in size of devices?

Look, I do not have time to argue with you forever and I do not even understand what is your point. Just go and read other filesystems implementation of FITRIM (ext4,ext3,xfs,btrfs?) and you'll see what you need to do.

If you do not want to get the file system size, then FINE! just pass the damn UULONG_MAX as length. I have no clue what insane loop are you talking about! It is *easy* just discard the whole thing (with UULONG_MAX) or, if you want to do it per-partes, then do it as long as it does not return EINVAL, once it does you know that your "start" is out of the filesystem and you are done!

Quoted 2 lines

Why can you guarantee it's not big deal in design? Why can't you admit userland can't make optimized loop?

And what do you mean by that ?

-Lukas

Quoted 9 lines · Lukas Czerner

Quoted 3 lines

What is size of file system or underlying devices? You force to find the device which target FS is using? Even if you can get size of underlying devices, you force to user to insane loop in size of devices?

Look, I do not have time to argue with you forever and I do not even understand what is your point. Just go and read other filesystems implementation of FITRIM (ext4,ext3,xfs,btrfs?) and you'll see what you need to do.

Quoted 6 lines

If you do not want to get the file system size, then FINE! just pass the damn UULONG_MAX as length. I have no clue what insane loop are you talking about! It is *easy* just discard the whole thing (with UULONG_MAX) or, if you want to do it per-partes, then do it as long as it does not return EINVAL, once it does you know that your "start" is out of the filesystem and you are done!

You are not even understanding current implementations. See ext3_trim_fs(), and ext4_trim_fs().

What happen if "start" was outside of max_blks:

ext3 returns 0
ext4 returns EINVAL

What means "start" is 0

ext3 maps to 1
ext4 just remove 0 from request

I missing something?

Quoted 6 lines

Quoted 2 lines

Why can you guarantee it's not big deal in design? Why can't you admit userland can't make optimized loop?

And what do you mean by that ?

-Lukas

--
OGAWA Hirofumi <[email protected]>

Hi Ogawa,

Do you still object to merge batched discard support on fat?

If the current batched discard concept doesn't changed, it can't merge it?

Quoted 1 line · Kyungmin Park

Hi Ogawa,

Hi,

Quoted 3 lines

Do you still object to merge batched discard support on fat?

If the current batched discard concept doesn't changed, it can't merge it?

I'm not blaming "concept". I'm saying current implements (and due to current interface confusion) is confusing and makes userland hard to use it.

I really think I explained all my concerns and current implement confusion.

Last time I checked, those implementations had even the different error code at EOF. How can I say your implement will work without knowing right implement?

Again, it is reason, and I'm not blaming "concept" or your implement itself.

Thanks.