Re: [PATCH 5/17] [LogFS] dir.c

Re: [PATCH 5/17] [LogFS] dir.c

[Dan Carpenter]

On Fri, Nov 20, 2009 at 08:37:29PM +0100, Joern Engel wrote:
> +static int logfs_unlink(struct inode *dir, struct dentry *dentry)
> +{
> +	struct logfs_super *super = logfs_super(dir->i_sb);
> +	struct inode *inode = dentry->d_inode;
> +	struct logfs_transaction *ta;
> +	struct page *page;
> +	pgoff_t index;
> +	int ret;
> +
> +	ta = kzalloc(sizeof(*ta), GFP_KERNEL);
> +	if (!ta)
> +		return -ENOMEM;
> +
> +	ta->state = UNLINK_1;
> +	ta->ino = inode->i_ino;
> +
> +	inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> +
> +	page = logfs_get_dd_page(dir, dentry);
> +	if (!page)

kfree(ta);

> +		return -ENOENT;
> +	if (IS_ERR(page))

kfree(ta);

> +		return PTR_ERR(page);

regards,
dan carpenter

Re: [PATCH 5/17] [LogFS] dir.c

[Jörn Engel]

On Mon, 23 November 2009 13:17:44 +0200, Dan Carpenter wrote:
> On Fri, Nov 20, 2009 at 08:37:29PM +0100, Joern Engel wrote:
> > +static int logfs_unlink(struct inode *dir, struct dentry *dentry)
> > +{
> > +	struct logfs_super *super = logfs_super(dir->i_sb);
> > +	struct inode *inode = dentry->d_inode;
> > +	struct logfs_transaction *ta;
> > +	struct page *page;
> > +	pgoff_t index;
> > +	int ret;
> > +
> > +	ta = kzalloc(sizeof(*ta), GFP_KERNEL);
> > +	if (!ta)
> > +		return -ENOMEM;
> > +
> > +	ta->state = UNLINK_1;
> > +	ta->ino = inode->i_ino;
> > +
> > +	inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME;
> > +
> > +	page = logfs_get_dd_page(dir, dentry);
> > +	if (!page)
> 
> kfree(ta);
> 
> > +		return -ENOENT;
> > +	if (IS_ERR(page))
> 
> kfree(ta);
> 
> > +		return PTR_ERR(page);

Ick!  The drunkard who wrote that code ought to get fired.  Thank you
for noticing, Dan.

Fixed in the git tree.  And maybe I should go look for similar cases.

Jörn

-- 
You can't tell where a program is going to spend its time. Bottlenecks
occur in surprising places, so don't try to second guess and put in a
speed hack until you've proven that's where the bottleneck is.
-- Rob Pike

Re: [PATCH 0/17] [LogFS] New flash filesystem

[Arnd Bergmann]

On Friday 20 November 2009, Joern Engel wrote:
> Mails are purely for review and comments.  If this is deemed
> merge-ready, please the git tree at
> http://git.kernel.org/?p=linux/kernel/git/joern/logfs.git
> instead.

I think merging is overdue, the code looks great and if you're
happy with it to go in, it's hard for anyone to argue with
that.

It should definitely be included in linux-next, and I guess it
should have been in there for some time, to get better
compile-time coverage across architectures, and some
more testing.

I can't find it in linux-next right now, but I could be
looking at the wrong stuff again, as I have before.

	Arnd <><

Re: [PATCH 0/17] [LogFS] New flash filesystem

[Jörn Engel]

On Mon, 23 November 2009 13:18:39 +0100, Arnd Bergmann wrote:
> On Friday 20 November 2009, Joern Engel wrote:
> > Mails are purely for review and comments.  If this is deemed
> > merge-ready, please the git tree at
> > http://git.kernel.org/?p=linux/kernel/git/joern/logfs.git
> > instead.
> 
> I think merging is overdue, the code looks great and if you're
> happy with it to go in, it's hard for anyone to argue with
> that.
> 
> It should definitely be included in linux-next, and I guess it
> should have been in there for some time, to get better
> compile-time coverage across architectures, and some
> more testing.
> 
> I can't find it in linux-next right now, but I could be
> looking at the wrong stuff again, as I have before.

Stephen,
do you have a reason not to add logfs to linux-next?

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

Re: [PATCH 0/17] [LogFS] New flash filesystem

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

Hi Jörn,

On Wed, 25 Nov 2009 16:55:11 +0100 Jörn Engel <joern@logfs.org> wrote:
>
> On Mon, 23 November 2009 13:18:39 +0100, Arnd Bergmann wrote:
> > 
> > It should definitely be included in linux-next, and I guess it
> > should have been in there for some time, to get better
> > compile-time coverage across architectures, and some
> > more testing.
> > 
> > I can't find it in linux-next right now, but I could be
> > looking at the wrong stuff again, as I have before.
> 
> do you have a reason not to add logfs to linux-next?

Apart from not having been asked to do so, no ;-)

i.e. send me a request with the git url and I will add it.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/17] [LogFS] New flash filesystem

[Jörn Engel]

On Thu, 26 November 2009 10:51:03 +1100, Stephen Rothwell wrote:
> On Wed, 25 Nov 2009 16:55:11 +0100 Jörn Engel <joern@logfs.org> wrote:
> > 
> > do you have a reason not to add logfs to linux-next?
> 
> Apart from not having been asked to do so, no ;-)
> 
> i.e. send me a request with the git url and I will add it.

Being of sound state of mind, I hereby request that you add
git://git.kernel.org/pub/scm/linux/kernel/git/joern/logfs.git
to linux-next.

Something like that?  Or would you doubt my state of mind if I willing
accept the risk of breaking others' compile and suffering the angry
emails that inevitably follow?

Jörn

-- 
A quarrel is quickly settled when deserted by one party; there is
no battle unless there be two.
-- Seneca

Re: [PATCH 0/17] [LogFS] New flash filesystem

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

Hi Jörn,

On Thu, 26 Nov 2009 09:36:15 +0100 Jörn Engel <joern@logfs.org> wrote:
>
> Being of sound state of mind, I hereby request that you add

:-)

> git://git.kernel.org/pub/scm/linux/kernel/git/joern/logfs.git
> to linux-next.
> 
> Something like that?  Or would you doubt my state of mind if I willing
> accept the risk of breaking others' compile and suffering the angry
> emails that inevitably follow?

Sometimes I question my own state of mind :-)

I have added the master branch of the above tree from today.

Thanks for adding your subsystem tree as a participant of linux-next.  As
you may know, this is not a judgment of your code.  The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window. 

You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
	Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary.

-- 
Cheers,
Stephen Rothwell 
sfr@canb.auug.org.au

Legal Stuff:
By participating in linux-next, your subsystem tree contributions are
public and will be included in the linux-next trees.  You may be sent
e-mail messages indicating errors or other issues when the
patches/commits from your subsystem tree are merged and tested in
linux-next.  These messages may also be cross-posted to the linux-next
mailing list, the linux-kernel mailing list, etc.  The linux-next tree
project and IBM (my employer) make no warranties regarding the linux-next
project, the testing procedures, the results, the e-mails, etc.  If you
don't agree to these ground rules, let me know and I'll remove your tree
from participation in linux-next.

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 12/17] [LogFS] readwrite.c

[Pekka Enberg]

Hi Joern,

(Dunno who to CC, really, so lets see if I can trick Andrew or Hugh
into looking at the issue.)

On Fri, Nov 20, 2009 at 9:38 PM, Joern Engel <joern@logfs.org> wrote:
> +static void logfs_lock_write_page(struct page *page)
> +{
> +       int loop = 0;
> +
> +       while (unlikely(!trylock_page(page))) {
> +               if (loop++ > 0x1000) {
> +                       /* Has been observed once so far... */
> +                       printk(KERN_ERR "stack at %p\n", &loop);
> +                       BUG();
> +               }
> +               if (PagePreLocked(page)) {
> +                       /* Holder of page lock is waiting for us, it
> +                        * is safe to use this page. */
> +                       break;
> +               }
> +               /* Some other process has this page locked and has
> +                * nothing to do with us.  Wait for it to finish.
> +                */
> +               schedule();
> +       }
> +       BUG_ON(!PageLocked(page));
> +}

What's the purpose of PagePreLocked()? The above function looks pretty
fragile for a filesystem to me.

                        Pekka

Re: [PATCH 12/17] [LogFS] readwrite.c

[Jörn Engel]

Hello Pekka!

On Mon, 23 November 2009 14:33:15 +0200, Pekka Enberg wrote:
> 
> (Dunno who to CC, really, so lets see if I can trick Andrew or Hugh
> into looking at the issue.)

That would be nice.

> On Fri, Nov 20, 2009 at 9:38 PM, Joern Engel <joern@logfs.org> wrote:
> > +static void logfs_lock_write_page(struct page *page)
> > +{
> > +       int loop = 0;
> > +
> > +       while (unlikely(!trylock_page(page))) {
> > +               if (loop++ > 0x1000) {
> > +                       /* Has been observed once so far... */
> > +                       printk(KERN_ERR "stack at %p\n", &loop);
> > +                       BUG();
> > +               }
> > +               if (PagePreLocked(page)) {
> > +                       /* Holder of page lock is waiting for us, it
> > +                        * is safe to use this page. */
> > +                       break;
> > +               }
> > +               /* Some other process has this page locked and has
> > +                * nothing to do with us.  Wait for it to finish.
> > +                */
> > +               schedule();
> > +       }
> > +       BUG_ON(!PageLocked(page));
> > +}
> 
> What's the purpose of PagePreLocked()? The above function looks pretty
> fragile for a filesystem to me.

Avoiding deadlocks.  Garbage collection is the cause of almost all
headaches in logfs, including this.  Any write may require some amount
of garbage collection to free up some space.  Garbage collection
consists of basically random reads followed by random writes.  So in
order to write any page, it may be required to first read and write some
other inconvenient page.

Simple case:
	Thread A writes page 1, GC then reads/writes page 1.
More complicated:
	Thread A writes page 1, thread B writes page 2.
	Thread A gets the write lock, thread B blocks on write lock.
	Thread A does GC which reads/writes page 2.

The more complicated case requires that any thread holding the write
lock must be able to read/write any page belonging to this filesystem.
If those pages are locked by someone else, it should wait it out and
only use the page when the page lock is released.  But if the pages are
locked in a logfs write path, that would cause a deadlock.

So PagePreLocked(page) indicates that this page is in a logfs write
path.  It won't change until the current thread releases the logfs write
lock and is fair game for GC.

Jörn

-- 
The cheapest, fastest and most reliable components of a computer
system are those that aren't there.
-- Gordon Bell, DEC labratories