FAT filesystem write and long names stability

FAT filesystem write and long names stability

[Robert Jarzmik]

Does anyone have a feedback about the FAT filesystem support in barebox ?

From my first tests, I think that :
 - long filenames is a bit buggy (when activated, I have a exception in a ls
 command)
 - write doesn't seem to work : on a mounted SD card, I make a "mkdir toto",
 then dismount and remount => the directory is gone.

Is anyone else doing tests in this area ?

Cheers.

-- 
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Franck JULLIEN]

Hi Robert,

2011/12/5 Robert Jarzmik <robert.jarzmik@free.fr>:
>
> Does anyone have a feedback about the FAT filesystem support in barebox ?
>
> From my first tests, I think that :
>  - long filenames is a bit buggy (when activated, I have a exception in a ls
>  command)
>  - write doesn't seem to work : on a mounted SD card, I make a "mkdir toto",
>  then dismount and remount => the directory is gone.
>
> Is anyone else doing tests in this area ?
>
> Cheers.
>
> --
> Robert
>
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox

Do you have this patch applied to your working branch ? :

fs/fat: Initialize local variable finfo

fat_stat in fs/fat.c declares finfo but doesn't initialize it.
When get_fileinfo is called, fno->lfname and fno->lfsize are
tested but haven't been zeroed...This can lead to a wrong
behavior.

When I worked on MMC SPI, I had problems with long file names. This
uninitialized
varible was the root cause...

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Robert Jarzmik]

Franck JULLIEN <franck.jullien@gmail.com> writes:

> Do you have this patch applied to your working branch ? :
>
> fs/fat: Initialize local variable finfo
Hi Franck,

I cross-checked, and indeed I do not have this fix in my branch, while it sits
in next branch. I'll rebase and make another try.

Do you by any chance tried the "write" part of FAT ?

Cheers.

-- 
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Franck JULLIEN]

2011/12/5 Robert Jarzmik <robert.jarzmik@free.fr>:
> Franck JULLIEN <franck.jullien@gmail.com> writes:
>
>> Do you have this patch applied to your working branch ? :
>>
>> fs/fat: Initialize local variable finfo
> Hi Franck,
>
> I cross-checked, and indeed I do not have this fix in my branch, while it sits
> in next branch. I'll rebase and make another try.
>
> Do you by any chance tried the "write" part of FAT ?
>
> Cheers.
>
> --
> Robert

Yes I did try it and as far as I can tell it works. I wrote some test
files to the SD then read it back on the PC and file were there....
I don't think I tried to create folders.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Robert Jarzmik]

Franck JULLIEN <franck.jullien@gmail.com> writes:

> 2011/12/5 Robert Jarzmik <robert.jarzmik@free.fr>:
>> Franck JULLIEN <franck.jullien@gmail.com> writes:
>>
>>> Do you have this patch applied to your working branch ? :
>>>
>>> fs/fat: Initialize local variable finfo
>> Hi Franck,
>>
>> I cross-checked, and indeed I do not have this fix in my branch, while it sits
>> in next branch. I'll rebase and make another try.
Good guess, with that patch no bug :)

>> Do you by any chance tried the "write" part of FAT ?
> Yes I did try it and as far as I can tell it works. I wrote some test
> files to the SD then read it back on the PC and file were there....
> I don't think I tried to create folders.
Ah that's the node of the story.
I made 2 tries :
 (1) mount the SD card, create a folder "toto", umount
     => when checked in my laptop, no new folder is created
 (2) mount the SD card, create a folder "toto", and copy in it a file "foo.txt",
 umount
     => when checked in my laptop, both the directory and the file *are* there

So I suppose that creating a directory without any file within doesn't trigger
the write on the device.

Thanks for your help.

Cheers.

-- 
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Sascha Hauer]

On Tue, Dec 06, 2011 at 11:01:37AM +0100, Robert Jarzmik wrote:
> Franck JULLIEN <franck.jullien@gmail.com> writes:
> 
> > 2011/12/5 Robert Jarzmik <robert.jarzmik@free.fr>:
> >> Franck JULLIEN <franck.jullien@gmail.com> writes:
> >>
> >>> Do you have this patch applied to your working branch ? :
> >>>
> >>> fs/fat: Initialize local variable finfo
> >> Hi Franck,
> >>
> >> I cross-checked, and indeed I do not have this fix in my branch, while it sits
> >> in next branch. I'll rebase and make another try.
> Good guess, with that patch no bug :)
> 
> >> Do you by any chance tried the "write" part of FAT ?
> > Yes I did try it and as far as I can tell it works. I wrote some test
> > files to the SD then read it back on the PC and file were there....
> > I don't think I tried to create folders.
> Ah that's the node of the story.
> I made 2 tries :
>  (1) mount the SD card, create a folder "toto", umount
>      => when checked in my laptop, no new folder is created
>  (2) mount the SD card, create a folder "toto", and copy in it a file "foo.txt",
>  umount
>      => when checked in my laptop, both the directory and the file *are* there
> 
> So I suppose that creating a directory without any file within doesn't trigger
> the write on the device.

I had a look at it and the problem seems to be the fat caching layer I
introduced. It brings the fat on disk and in memory out of sync. This
layer has been necessary since without it the performance of the fat
driver is really poor. You could try the following branch. It reverts
the fat cache layer support and instead reimplements the block caching
layer so that it can handle the access patterns of the fat driver
better.
It would be great if you could give it some testing as obviously my test
patterns didn't reveal the 'create folder' bug you described.

Sascha

The following changes since commit 3bb6ee8dd530d01724ceb7c3d5bb68bd1898726a:

  mci: add the probe parameter if any error happened during the probe (2011-12-05 22:05:09 +0100)

are available in the git repository at:
  git://git.pengutronix.de/git/barebox.git pu/block

Sascha Hauer (3):
      list: add list_last_entry function
      fat: revert fat caching mechanism
      block: reimplement caching

 common/block.c       |  274 ++++++++++++++++++++++++++++++++++++-------------
 fs/fat/ff.c          |   93 ++++-------------
 include/block.h      |   16 ++--
 include/linux/list.h |   11 ++
 4 files changed, 241 insertions(+), 153 deletions(-)
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Robert Jarzmik]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> On Tue, Dec 06, 2011 at 11:01:37AM +0100, Robert Jarzmik wrote:
> I had a look at it and the problem seems to be the fat caching layer I
> introduced. It brings the fat on disk and in memory out of sync. This
> layer has been necessary since without it the performance of the fat
> driver is really poor. You could try the following branch. It reverts
> the fat cache layer support and instead reimplements the block caching
> layer so that it can handle the access patterns of the fat driver
> better.
> It would be great if you could give it some testing as obviously my test
> patterns didn't reveal the 'create folder' bug you described.
Tomorrow or wednesday, I'll make the few tests I always do (making a
directory, copying a file, deleting a file, etc ...) and report.

As I also happen to use the FAT on SDCard for reading my framebuffer image, and
also to drop MTD content onto the same FAT, I'll tell you my "feeling" about
performance.

Cheers.

--
Robert

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Robert Jarzmik]

Robert Jarzmik <robert.jarzmik@free.fr> writes:

> Sascha Hauer <s.hauer@pengutronix.de> writes:
>> It would be great if you could give it some testing as obviously my test
>> patterns didn't reveal the 'create folder' bug you described.
> Tomorrow or wednesday, I'll make the few tests I always do (making a
> directory, copying a file, deleting a file, etc ...) and report.
This is what my tests give back:

**** With FAT caching (barebox.git/next + rjk mioa701 support):
barebox:/sdcard time bmp /sdcard/help.bmp 
time: 6895ms
barebox:/sdcard time cp /sdcard/help.bmp /sdcard/toto.tmp
time: 13278ms
barebox:/sdcard ls -l help.bmp
-rwxrwxrwx     230454 help.bmp
barebox:/sdcard 

**** Without FAT caching (merged pu/block into barebox.git/next + rjk mioa701 support)
barebox:/sdcard time bmp /sdcard/help.bmp
time: 9219ms
barebox:/sdcard time cp /sdcard/help.bmp /sdcard/titi.tmp
BUG: failure at common/block.c:248/block_

The "BUG" thing prevents me from going further. But it seems the cache in main
tree speeds up things. I also think that creating directories without files
serves no purpose I can think of, and is an acceptable tradeoff for the improved
performance.

Cheers.

--
Robert

PS: help.bmp is a converted bmp, 320x480, 24 bpp from :
    http://t0.gstatic.com/images?q=tbn:ANd9GcQssQd1VBKJUde7lZpU49GVwDaZbNGvbDNbrOeSbPrL_MfBkK61SA

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Sascha Hauer]

On Thu, Dec 15, 2011 at 10:28:38AM +0100, Robert Jarzmik wrote:
> Robert Jarzmik <robert.jarzmik@free.fr> writes:
> 
> > Sascha Hauer <s.hauer@pengutronix.de> writes:
> >> It would be great if you could give it some testing as obviously my test
> >> patterns didn't reveal the 'create folder' bug you described.
> > Tomorrow or wednesday, I'll make the few tests I always do (making a
> > directory, copying a file, deleting a file, etc ...) and report.
> This is what my tests give back:
> 
> **** With FAT caching (barebox.git/next + rjk mioa701 support):
> barebox:/sdcard time bmp /sdcard/help.bmp 
> time: 6895ms
> barebox:/sdcard time cp /sdcard/help.bmp /sdcard/toto.tmp
> time: 13278ms
> barebox:/sdcard ls -l help.bmp
> -rwxrwxrwx     230454 help.bmp
> barebox:/sdcard 

There is something wrong. These times are far from being acceptable.

Here's what I get with copying a 2MiB file from SD Card:

barebox@Freescale i.MX51 PDK:/ time cp /fat/x /x
time: 273ms

> 
> **** Without FAT caching (merged pu/block into barebox.git/next + rjk mioa701 support)
> barebox:/sdcard time bmp /sdcard/help.bmp
> time: 9219ms
> barebox:/sdcard time cp /sdcard/help.bmp /sdcard/titi.tmp
> BUG: failure at common/block.c:248/block_

Hm, I don't understand where this can come from.

	data = block_get(blk, block);
	if (!data)
		BUG();

So this bug is triggered when block_get fails:

static void *block_get(struct block_device *blk, int block)
{
	void *outdata;
	int ret;

	if (block >= blk->num_blocks)
		return NULL;

	outdata = block_get_cached(blk, block);
	if (outdata)
		return outdata;

	ret = block_cache(blk, block);
	if (ret)
		return NULL;

	outdata = block_get_cached(blk, block);
	if (!outdata)
		BUG();

	return outdata;
}

So block_get fails when

a) You access some block outside the device (which should have been
   caught earlier
b) block_cache fails. (This indeed can fail when the underlying hardware
   fails to read the block, so the BUG should be replaced with a simple
   error return)

> 
> The "BUG" thing prevents me from going further. But it seems the cache in main
> tree speeds up things. I also think that creating directories without files
> serves no purpose I can think of, and is an acceptable tradeoff for the improved
> performance.

It's not only the creating-directory-without-files thing, The FAT can
become corrupted in current mainline, so we have to do something.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: FAT filesystem write and long names stability

[Sascha Hauer]

On Mon, Dec 19, 2011 at 12:01:20PM +0100, Sascha Hauer wrote:
> On Thu, Dec 15, 2011 at 10:28:38AM +0100, Robert Jarzmik wrote:
> > Robert Jarzmik <robert.jarzmik@free.fr> writes:
> > 
> > > Sascha Hauer <s.hauer@pengutronix.de> writes:
> > >> It would be great if you could give it some testing as obviously my test
> > >> patterns didn't reveal the 'create folder' bug you described.
> > > Tomorrow or wednesday, I'll make the few tests I always do (making a
> > > directory, copying a file, deleting a file, etc ...) and report.
> > This is what my tests give back:
> > 
> > **** With FAT caching (barebox.git/next + rjk mioa701 support):
> > barebox:/sdcard time bmp /sdcard/help.bmp 
> > time: 6895ms
> > barebox:/sdcard time cp /sdcard/help.bmp /sdcard/toto.tmp
> > time: 13278ms
> > barebox:/sdcard ls -l help.bmp
> > -rwxrwxrwx     230454 help.bmp
> > barebox:/sdcard 
> 
> There is something wrong. These times are far from being acceptable.

BTW Does your SD card work with a proper frequency?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] drivers/mci: pxa fix clockrate

[Robert Jarzmik]

The clock rate was incorrectly calculated, leading to a
frequency of 19.5MHz / 64 instead of 19.5Mz for the host
controller.

with the fix applied, a copy of a file of 230 kB shrinks
from 6000ms to 123ms.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mci/pxamci.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/mci/pxamci.c b/drivers/mci/pxamci.c
index 1634a1d..75b61f0 100644
--- a/drivers/mci/pxamci.c
+++ b/drivers/mci/pxamci.c
@@ -161,7 +161,8 @@ static int pxamci_transfer_data(struct pxamci_host *host,
 static void pxamci_start_cmd(struct pxamci_host *host, struct mci_cmd *cmd,
 			     unsigned int cmdat)
 {
-	mci_dbg("cmd=(idx=%d,type=%d)\n", cmd->cmdidx, cmd->resp_type);
+	mci_dbg("cmd=(idx=%d,type=%d,clkrt=%d)\n", cmd->cmdidx, cmd->resp_type,
+		host->clkrt);
 	if (cmd->resp_type & MMC_RSP_BUSY)
 		cmdat |= CMDAT_BUSY;
 
@@ -277,11 +278,14 @@ static void pxamci_set_ios(struct mci_host *mci, struct device_d *dev,
 {
 	struct pxamci_host *host = to_pxamci(mci);
 	unsigned int clk_in = pxa_get_mmcclk();
-	unsigned long fact;
+	int fact;
 
-	mci_dbg("bus_width=%d, clock=%ud\n", bus_width, clock);
-	fact = min_t(int, clock / clk_in, 1);
-	fact = max_t(int, fact, 1 << 6);
+	mci_dbg("bus_width=%d, clock=%u\n", bus_width, clock);
+	if (clock)
+		fact = min_t(int, clk_in / clock, 1 << 6);
+	else
+		fact = 1 << 6;
+	fact = max_t(int, fact, 1);
 
 	/*
 	 * We calculate clkrt here, and will write it on the next command
-- 
1.7.5.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox