From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kyungmin Park Subject: Re: [PATCH] fat: Batched discard support for fat Date: Thu, 17 Feb 2011 20:01:20 +0900 Message-ID: References: <20110217052220.GA1372@july> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: OGAWA Hirofumi , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org To: Lukas Czerner Return-path: Received: from mail-gy0-f174.google.com ([209.85.160.174]:58400 "EHLO mail-gy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751833Ab1BQLBV convert rfc822-to-8bit (ORCPT ); Thu, 17 Feb 2011 06:01:21 -0500 In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Feb 17, 2011 at 7:48 PM, Lukas Czerner wr= ote: > On Thu, 17 Feb 2011, Kyungmin Park wrote: > >> From: Kyungmin Park >> >> FAT supports batched discard as ext4. >> >> Cited from Lukas words. >> "The current solution is not ideal because of its bad performance im= pact. >> So basic idea to improve things is to avoid discarding every time so= me >> blocks are freed. and instead batching is together into bigger trims= , >> which tends to be more effective." >> >> You can find an information in detail at following URLs. >> http://lwn.net/Articles/397538/ >> http://lwn.net/Articles/383933/ > > Hi Kyungmin, > > this is really great to see more and more filesystemS ADDING this. I = can > not really comment on fat specific code, but anyway I have couple of > comments bellow. > > Thanks! > -Lukas > >> >> Signed-off-by: Kyungmin Park >> --- >> diff --git a/fs/fat/fat.h b/fs/fat/fat.h >> index f504089..08b53e1 100644 >> --- a/fs/fat/fat.h >> +++ b/fs/fat/fat.h >> @@ -299,6 +299,7 @@ extern int fat_alloc_clusters(struct inode *inod= e, int *cluster, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int nr_clust= er); >> =A0extern int fat_free_clusters(struct inode *inode, int cluster); >> =A0extern int fat_count_free_clusters(struct super_block *sb); >> +extern int fat_trim_fs(struct super_block *sb, struct fstrim_range = *range); >> >> =A0/* fat/file.c */ >> =A0extern long fat_generic_ioctl(struct file *filp, unsigned int cmd= , >> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c >> index b47d2c9..777094b 100644 >> --- a/fs/fat/fatent.c >> +++ b/fs/fat/fatent.c >> @@ -1,6 +1,8 @@ >> =A0/* >> =A0 * Copyright (C) 2004, OGAWA Hirofumi >> =A0 * Released under GPL v2. >> + * >> + * Batched discard support by Kyungmin Park >> =A0 */ >> >> =A0#include >> @@ -541,6 +543,16 @@ out: >> =A0 =A0 =A0 return err; >> =A0} >> >> +static int fat_issue_discard(struct super_block *sb, int cluster, i= nt nr_clus) >> +{ >> + =A0 =A0 struct msdos_sb_info *sbi =3D MSDOS_SB(sb); >> + =A0 =A0 sector_t block, nr_blocks; >> + >> + =A0 =A0 =A0 =A0block =3D fat_clus_to_blknr(sbi, cluster); >> + =A0 =A0 =A0 =A0nr_blocks =3D nr_clus * sbi->sec_per_clus; >> + =A0 =A0 =A0 =A0return sb_issue_discard(sb, block, nr_blocks, GFP_N= OFS, 0); > > Use tabs for code indent. > >> +} >> + >> =A0int fat_free_clusters(struct inode *inode, int cluster) >> =A0{ >> =A0 =A0 =A0 struct super_block *sb =3D inode->i_sb; >> @@ -575,11 +587,7 @@ int fat_free_clusters(struct inode *inode, int = cluster) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (cluster !=3D fatent.= entry + 1) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int nr_c= lus =3D fatent.entry - first_cl + 1; >> >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 sb_issue_d= iscard(sb, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 fat_clus_to_blknr(sbi, first_cl), >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 nr_clus * sbi->sec_per_clus, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 GFP_NOFS, 0); >> - >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fat_issue_= discard(sb, first_cl, nr_clus); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 first_cl= =3D cluster; >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> @@ -683,3 +691,73 @@ out: >> =A0 =A0 =A0 unlock_fat(sbi); >> =A0 =A0 =A0 return err; >> =A0} >> + >> +int fat_trim_fs(struct super_block *sb, struct fstrim_range *range) >> +{ >> + =A0 =A0 struct msdos_sb_info *sbi =3D MSDOS_SB(sb); >> + =A0 =A0 struct fatent_operations *ops =3D sbi->fatent_ops; >> + =A0 =A0 struct fat_entry fatent; >> + =A0 =A0 unsigned long reada_blocks, reada_mask, cur_block; >> + =A0 =A0 int err =3D 0, free; >> + =A0 =A0 unsigned int start, len, minlen, trimmed; >> + =A0 =A0 int entry =3D 0; >> + >> + =A0 =A0 start =3D range->start >> sb->s_blocksize_bits; > > Start is not used anywhere either. Let me just explain the reason for > having start, and len. > > start - address the first Byte of the filesystem (or better, the devi= ce, > =A0or part of the device underneath the filesystem itself). You are > =A0supposed to use this as a starting point for doing discard. > > len - defines the length in Bytes of the filesystem (or better, the > =A0device, or part of the device underneath the filesystem itself), u= ser > =A0want to discard from the "start". Good explanation. yes I know it's not used at current patch but it's added for ext4 does. If anyone complaint it, I can remove it for this time. > > This two values gives us more flexibility in the way we are discardin= g > the filesystem's free blocks. For example, if you have huge filesyste= m, > or your device has bad discard performance, you probably do not want = to > do the FITRIM all-at-once, but rather per-partes, to not disturb othe= r > ongoing IO too much. Basically it allows you to spread the load throu= gh > longer period of time. > > Also, for bigger filesystems we might want to inform user that someth= ing > is really happening in the form of progress bar (or whatever), which = you > can not do otherwise. > >> + =A0 =A0 len =3D range->len >> sb->s_blocksize_bits; > > "len" does not seem to be used anywhere, does it mean that you are > discarding free extents on the whole filesystem with one run ? This > operation can take pretty long time on devices with slow discard > capability. Can you consider doing it per-partes as we are doing in > ext4 for example ? I really can't say how much work does it mean in > fat, but it might be worth it. Especially because of ... -> > >> + =A0 =A0 minlen =3D range->minlen >> sb->s_blocksize_bits; >> + =A0 =A0 trimmed =3D 0; >> + >> + =A0 =A0 minlen =3D minlen / sbi->sec_per_clus; >> + >> + =A0 =A0 lock_fat(sbi); > =A0 =A0 =A0 =A0^^^^^^^^^^^^^^ > =A0 =A0 =A0 =A0-> this > You are holding this mutex the whole time you are discarding free > extents and I think it would be devastating, because the users will s= ee > long stalls. The bigger the filesystem is and the worse discard > performance of the device is, the longer stall will be. Please correc= t > me I am mistaken. Have you done any testing ? Now I assume it's not perform during fat operation at normal case. It's performed at specific time instead of parallel IO as ext4 does. e.g., it's performed after disconnected from windows and so on. another reason is can't guarantee the fat table entry consistency during trim, I just lock the fat at start time as count the free space at fat codes. > >> + =A0 =A0 if (sbi->free_clusters !=3D -1 && sbi->free_clus_valid) >> + =A0 =A0 =A0 =A0 =A0 =A0 goto out; >> + >> + =A0 =A0 reada_blocks =3D FAT_READA_SIZE >> sb->s_blocksize_bits; >> + =A0 =A0 reada_mask =3D reada_blocks - 1; >> + =A0 =A0 cur_block =3D 0; >> + >> + =A0 =A0 free =3D 0; >> + =A0 =A0 fatent_init(&fatent); >> + >> + =A0 =A0 /* >> + =A0 =A0 =A0* REVISIT: scan from the last free block. >> + =A0 =A0 =A0*/ >> + =A0 =A0 fatent_set_entry(&fatent, FAT_START_ENT); >> + =A0 =A0 while (fatent.entry < sbi->max_cluster) { >> + =A0 =A0 =A0 =A0 =A0 =A0 /* readahead of fat blocks */ >> + =A0 =A0 =A0 =A0 =A0 =A0 if ((cur_block & reada_mask) =3D=3D 0) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 unsigned long rest =3D sbi= ->fat_length - cur_block; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fat_ent_reada(sb, &fatent,= min(reada_blocks, rest)); >> + =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 cur_block++; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D fat_ent_read_block(sb, &fatent); >> + =A0 =A0 =A0 =A0 =A0 =A0 if (err) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 do { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ops->ent_get(&fatent) = =3D=3D FAT_ENT_FREE) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 free++; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!entry= ) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 entry =3D fatent.entry; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else if (entry) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (free >= =3D minlen) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 fat_issue_discard(sb, entry, free); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 trimmed +=3D free; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 free =3D 0= ; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 entry =3D = 0; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 } while (fat_ent_next(sbi, &fatent)); >> + =A0 =A0 } >> + =A0 =A0 if (free >=3D minlen) { >> + =A0 =A0 =A0 =A0 =A0 =A0 fat_issue_discard(sb, entry, free); >> + =A0 =A0 =A0 =A0 =A0 =A0 trimmed +=3D free; >> + =A0 =A0 } >> + =A0 =A0 range->len =3D (trimmed * sbi->sec_per_clus) * sb->s_block= size; >> + =A0 =A0 fatent_brelse(&fatent); >> +out: >> + =A0 =A0 unlock_fat(sbi); >> + =A0 =A0 return err; >> +} >> diff --git a/fs/fat/file.c b/fs/fat/file.c >> index 7257752..bfdd558 100644 >> --- a/fs/fat/file.c >> +++ b/fs/fat/file.c >> @@ -125,6 +125,30 @@ long fat_generic_ioctl(struct file *filp, unsig= ned int cmd, unsigned long arg) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return fat_ioctl_get_attributes(inode, u= ser_attr); >> =A0 =A0 =A0 case FAT_IOCTL_SET_ATTRIBUTES: >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return fat_ioctl_set_attributes(filp, us= er_attr); >> + =A0 =A0 case FITRIM: >> + =A0 =A0 { >> + =A0 =A0 =A0 =A0 =A0 =A0 struct super_block *sb =3D inode->i_sb; >> + =A0 =A0 =A0 =A0 =A0 =A0 struct fstrim_range range; >> + =A0 =A0 =A0 =A0 =A0 =A0 int ret =3D 0; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 if (!capable(CAP_SYS_ADMIN)) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EPERM; > > You might want to add check whether the device actually support disca= rd. > See: http://www.spinics.net/lists/linux-ext4/msg23144.html Good. I didn't see the patch, next version I'll add it. Thank you, Kyungmin Park > >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 if (copy_from_user(&range, (struct fstrim_= range *)arg, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 sizeof(range))) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EFAULT; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 ret =3D fat_trim_fs(sb, &range); >> + =A0 =A0 =A0 =A0 =A0 =A0 if (ret < 0) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 if (copy_to_user((struct fstrim_range *)ar= g, &range, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 sizeof(range))) >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EFAULT; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 return 0; >> + =A0 =A0 } >> + >> =A0 =A0 =A0 default: >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOTTY; /* Inappropriate ioctl f= or device */ >> =A0 =A0 =A0 } >> > > -- > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html