From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C27F3C43334 for ; Thu, 14 Jul 2022 00:39:39 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 0226A80C75; Thu, 14 Jul 2022 02:39:37 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="cY7rYiar"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id BEDCC810FE; Thu, 14 Jul 2022 02:39:34 +0200 (CEST) Received: from mail-pj1-x1031.google.com (mail-pj1-x1031.google.com [IPv6:2607:f8b0:4864:20::1031]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id ED8D480112 for ; Thu, 14 Jul 2022 02:39:30 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pj1-x1031.google.com with SMTP id 89-20020a17090a09e200b001ef7638e536so6442779pjo.3 for ; Wed, 13 Jul 2022 17:39:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=Re/eIpC+Nn7Vcu5sDqgd0o5AcFNr+IYKgL/zE9N4S54=; b=cY7rYiarZrokS8UPHIjIEnaMqOosDvc4LL45p8xOJ2vvdpHTXUdxyZuEW1PbW3hgpt l3WImjPuK7x9CQesSNDGf/Krt9fcC+Bft6PMCe950h18bVfTM+zKhcmXQgrOJtI3eQT/ 8cJLaxIO6PdExDyZf4XMS6iIDP46xyWGf2g7B3+BkHVfWyO/5jx2hkqO9T5y3/ZZP10b JFj8JdIyYdybxyifGYkLpBOrXHm/Pf3Xf9J5josR8b3ViPs+0LE9uZUoefKeMEOyLtLy IW+DtGQTYQWn0OYVs1B3pbXAEuXR5XSPt21z8B+4+zkB3Hpg6p1/PO1Hvj+EuiGH9Mf3 cbUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=Re/eIpC+Nn7Vcu5sDqgd0o5AcFNr+IYKgL/zE9N4S54=; b=3xwBhITlFqgOFrbr1XEcNtHnuYNbNLhbspiAMfnADiS6tu6PwQl6Sr7KiUydmUhOao HtRJZj6XTgT2kDefQ4H68Dya8Wc32HkAmjjisefc+e5g0C1dfZQaui8g1WEZFDmdlLwL kDIjJVzz94zEiIaVjKRld0/XhRm9l25qGL2pbsrev/0gBnC2LWge3DljVI3XQJYgr8Kf wK/pmEZ0L7+LDPYqRTDyE4jt+Vx3oqoBlkOGqQlj8CLBuZgoVu7tiMsLVHncZsfvYeuH JnWz60+DAmwTIMt95+vqJNZ+j86NaV3WmUS2rj1SPZ3bm+O8ze2GD7ncgnGf2VzHssIk YXKA== X-Gm-Message-State: AJIora9zk7tS6VZpTZ+kBEcI5YntVDMGmV6hkxksJDKOnBcm3+PMfkGr 6W4Ev82EITXIQ6m1hX31eEJ3Xw== X-Google-Smtp-Source: AGRyM1s+L94R11YwBpYTK46Q54W0032cHZypMF7t/pr1Jzhwg3FJqXoAClzMQ1fGpVXOKDnYemTKGA== X-Received: by 2002:a17:903:1108:b0:16a:a0ab:8f89 with SMTP id n8-20020a170903110800b0016aa0ab8f89mr5712541plh.12.1657759168919; Wed, 13 Jul 2022 17:39:28 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:c57c:842f:2db2:bf43]) by smtp.gmail.com with ESMTPSA id 22-20020a17090a0e9600b001ef88c30fbbsm2238840pjx.49.2022.07.13.17.39.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Jul 2022 17:39:28 -0700 (PDT) Date: Thu, 14 Jul 2022 09:39:25 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: Tom Rini , u-boot@lists.denx.de Subject: Re: [PATCH 1/3] fs: fat: finding an empty FAT cluster Message-ID: <20220714003925.GA42596@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , Tom Rini , u-boot@lists.denx.de References: <20220712223314.20530-1-xypron.glpk@gmx.de> <20220712223314.20530-2-xypron.glpk@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220712223314.20530-2-xypron.glpk@gmx.de> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Thank you for fixing this. On Tue, Jul 12, 2022 at 10:33:12PM +0000, Heinrich Schuchardt wrote: > Currently we have two functions with redundant coding to find an empty > cluster: > > * find_empty_cluster() seeks from the beginning of the FAT table > * determine_fatent() seeks after a given entry > > Both do not detect the end of the FAT table correctly and return an invalid > cluster number if no empty entry if found. > > find_empty_cluster() is replaced by an invocation of determine_fatent(). > > determine_fatent() is changed to seek in a second round from the beginning > of the FAT table and to return an error code if no free entry is found. > With this patch we will always find an empty cluster if it exists. nitpick: empty -> free for consistent uses across this patch > Further patches are needed to handle the disk full error gracefully. > > Signed-off-by: Heinrich Schuchardt > --- > fs/fat/fat_write.c | 56 ++++++++++++++++++++++++---------------------- > 1 file changed, 29 insertions(+), 27 deletions(-) > > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c > index 8ff2f6def0..a137e14f41 100644 > --- a/fs/fat/fat_write.c > +++ b/fs/fat/fat_write.c > @@ -536,22 +536,41 @@ static int set_fatent_value(fsdata *mydata, __u32 entry, __u32 entry_value) > return 0; > } > > -/* > - * Determine the next free cluster after 'entry' in a FAT (12/16/32) table > - * and link it to 'entry'. EOC marker is not set on returned entry. > +/** > + * determine_fatent() - get next free FAT cluster > + * > + * The parameter @entry indicates the current cluster. To reduce fragementation > + * the function first searches for a free cluster after the current cluster. > + * If none is found, the search is repeated from the beginning of the FAT table. > + * > + * If @entry is set, the new FAT entry is appended to the given one. > + * If @entry is zero, only the number of the first free cluster is returned. > + * > + * @entry: current entry > + * Return: next free cluster or negative error > */ > -static __u32 determine_fatent(fsdata *mydata, __u32 entry) > +static int determine_fatent(fsdata *mydata, __u32 entry) > { > - __u32 next_fat, next_entry = entry + 1; > + __u32 next_fat, next_entry = entry; > + int second_round = 0; > > while (1) { > + ++next_entry; > + if (CHECK_CLUST(next_entry, mydata->fatsize)) { > + if (!second_round) { > + second_round = 1; > + next_entry = 3; > + } else { > + return -ENOSPC; > + } > + } The logic is fine, but in case of entry != 0 and second_round and if disk is full, fat entries after "entry" can unnecessarily be checked twice. So, next_entry = entry + 1; while (1) { if (next_entry == entry) return -ENOSPC; if (CHECK_CLUST(...)) { if (!entry) return -ENOSPC; next_entry = 3; continue; } ... next_entry++; } should work better. -Takahiro Akashi > next_fat = get_fatent(mydata, next_entry); > - if (next_fat == 0) { > + if (!next_fat) { > /* found free entry, link to entry */ > - set_fatent_value(mydata, entry, next_entry); > + if (entry) > + set_fatent_value(mydata, entry, next_entry); > break; > } > - next_entry++; > } > debug("FAT%d: entry: %08x, entry_value: %04x\n", > mydata->fatsize, entry, next_entry); > @@ -794,23 +813,6 @@ get_set_cluster(fsdata *mydata, __u32 clustnum, loff_t pos, __u8 *buffer, > return 0; > } > > -/* > - * Find the first empty cluster > - */ > -static int find_empty_cluster(fsdata *mydata) > -{ > - __u32 fat_val, entry = 3; > - > - while (1) { > - fat_val = get_fatent(mydata, entry); > - if (fat_val == 0) > - break; > - entry++; > - } > - > - return entry; > -} > - > /** > * new_dir_table() - allocate a cluster for additional directory entries > * > @@ -824,7 +826,7 @@ static int new_dir_table(fat_itr *itr) > int dir_oldclust = itr->clust; > unsigned int bytesperclust = mydata->clust_size * mydata->sect_size; > > - dir_newclust = find_empty_cluster(mydata); > + dir_newclust = determine_fatent(mydata, 0); > > /* > * Flush before updating FAT to ensure valid directory structure > @@ -1066,7 +1068,7 @@ set_clusters: > > /* Assure that curclust is valid */ > if (!curclust) { > - curclust = find_empty_cluster(mydata); > + curclust = determine_fatent(mydata, 0); > set_start_cluster(mydata, dentptr, curclust); > } else { > newclust = get_fatent(mydata, curclust); > -- > 2.30.2 >