lkml.org 
[lkml]   [2011]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] fat: Batched discard support for fat
On Thu, 24 Feb 2011, Kyungmin Park wrote:

...snip...
> >> >> +     while (count < sbi->max_cluster) {
> >> >> +             if (fatent.entry >= sbi->max_cluster)
> >> >> +                     fatent.entry = FAT_START_ENT;
> >> >> +             /* readahead of fat blocks */
> >> >> +             if ((cur_block & reada_mask) == 0) {
> >> >> +                     unsigned long rest = sbi->fat_length - cur_block;
> >> >> +                     fat_ent_reada(sb, &fatent, min(reada_blocks, rest));
> >> >> +             }
> >> >> +             cur_block++;
> >> >> +
> >> >> +             err = fat_ent_read_block(sb, &fatent);
> >> >> +             if (err)
> >> >> +                     goto out;
> >> >> +
> >> >> +             do {
> >> >> +                     if (ops->ent_get(&fatent) == FAT_ENT_FREE) {
> >> >> +                             free++;
> >> >> +                             if (!entry)
> >> >> +                                     entry = fatent.entry;
> >> >> +                             if (free >= (len - trimmed) && free >= minlen) {
> >> >
> >> > It seems to me that you are using len as number of bytes to trim. This
> >> > is not right and I am sorry for not explaining it correctly. "len" is
> >> > supposed to be a number of bytes you want to "investigate" from the start.
> >> > So it means that you will trim every single free extent bigger than minlen
> >> > between "start" byte and "start + len" byte of the underlying device, or
> >> > partition.
> >> No. len is adjusted at fat cluster number. it's not used byte unit.
> >> I think it's easy to compare with fat internal units.
> >
> > Does not matter what units are you using for len. I it just that you are
> > checking for (free >= (len - trimmed)) which is wrong because len is not
> > meant to be "overall length of trimmed data" but rather "overall length
> > of data to walk through and check for free extents" see ext4
> > implementation for reference.
> I think I used it as you said. e.g., I want to trim 256 (* minimum
> discard granularity),
> First time, I can find 10 entries. and trimmed has 10 and len has still 256.
> next time, I found the 246 free entries then trim remaining 246 one.
>
> do you want to trim it more than given len?

Let the "O" be free (bytes, blocks, whatever), and "=" be used.
Now, we have a filesystem like this.

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

This is how it supposed to wotk if you have called FITIRM with parameters:

start = 0
minlen = 2
len = 20

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


But in your implementation you will trim extents:
0-3
10-11
15-19
30-36

Hope it is clear now.

>
> Thank you,
> Kyungmin Park
> >
> > Thanks!
> > -Lukas
> >
> >>
> >> I hope fat peoples comment this one.
> >>
> >> Thank you,
> >> Kyungmin Park
> >> >
> >> >> +                                     fat_issue_discard(sb, entry, free);
> >> >> +                                     trimmed += free;
> >> >> +                             }
> >> >> +                             if (trimmed >= len)
> >> >> +                                     goto done;
> >> >> +                     } else if (entry) {
> >> >> +                             if (free >= minlen) {
> >> >> +                                     fat_issue_discard(sb, entry, free);
> >> >> +                                     trimmed += free;
> >> >> +                             }
> >> >> +                             if (trimmed >= len)
> >> >> +                                     goto done;
> >> >> +                             free = 0;
> >> >> +                             entry = 0;
> >> >> +                     }
> >> >> +                     count++;
> >> >> +             } while (fat_ent_next(sbi, &fatent));
> >> >> +     }
> >> >> +     if (free >= minlen) {
> >> >> +             fat_issue_discard(sb, entry, free);
> >> >> +             trimmed += free;
> >> >> +     }
> >> >> +done:
> >> >> +     range->len = (trimmed * sbi->sec_per_clus) << sb->s_blocksize_bits;
> >> >> +     fatent_brelse(&fatent);
> >> >> +out:
> >> >> +     unlock_fat(sbi);
> >> >> +     return err;
> >> >> +}

...snip...
\
 
 \ /
  Last update: 2011-02-24 12:43    [W:0.037 / U:1.940 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site