All of lore.kernel.org
 help / color / mirror / Atom feed
From: Boaz Harrosh <boaz@plexistor.com>
To: Tejun Heo <tj@kernel.org>, Boaz Harrosh <boaz@plexistor.com>
Cc: Jens Axboe <axboe@kernel.dk>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Christoph Hellwig <hch@infradead.org>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices
Date: Thu, 20 Nov 2014 16:14:29 +0200	[thread overview]
Message-ID: <546DF745.1070901@plexistor.com> (raw)
In-Reply-To: <20141120131118.GB14877@htj.dyndns.org>

On 11/20/2014 03:11 PM, Tejun Heo wrote:
> Hello, Boaz.
> 
<>
> W/ preloading, one way to do it is,
> 
> 	if (preload())
> 		handle -ENOMEM;
> 	lock;
> 	error = insert();
> 	if (error)
> 		handle error which can't be -ENOMEM;
> 	unlock;
> 	preload_end();
> 

I like this one, cause of the place I come from. Where
in a cluster you want the local fails as early as possible
before you start to commit remotely, and need to undo on
errors.

And I can see the real flow of things

> Another way is
> 
> 	preload();	// can't fail
> 	lock;
> 	error = insert();
> 	if (error)
> 		handle error;'
> 	unlock;
> 	preload_end();
> 
> Both ways have pros and cons.  The latter seems to lead to simpler
> code in general.  Not always, but the overall.
> 

I still like the over all simplicity of the above pattern to
this behind the seen complexity hidden away under the carpet.

But I guess that is just me. That is your call sir.

I do see your point though.

<>
> 
> And that's why the pattern usually leads to simpler code - it doesn't
> create a new failure point.
> 

Again a matter of taste. I like the extra ENOMEM failure point before
I started to commit to any state changes, lock grabbing and unrolling
in case of errors.

But I see your points as well. For what it is worth I have reviewed
your code and did not find any faults in it. It looks like sound
code.

Thanks
Boaz

  parent reply	other threads:[~2014-11-20 14:14 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-13 22:09 [PATCH vfs 1/2] lib: implement ptrset Tejun Heo
2014-11-13 22:11 ` [PATCH vfs 2/2] {block|char}_dev: remove inode->i_devices Tejun Heo
2014-11-18 12:10   ` Boaz Harrosh
2014-11-18 12:30     ` Tejun Heo
2014-11-20 10:42   ` Boaz Harrosh
2014-11-20 11:50     ` Tejun Heo
2014-11-20 12:36       ` Boaz Harrosh
2014-11-20 13:11         ` Tejun Heo
2014-11-20 13:39           ` Tejun Heo
2014-11-20 14:14           ` Boaz Harrosh [this message]
2014-11-20 14:19             ` Tejun Heo
2014-11-13 22:23 ` [PATCH vfs 1/2] lib: implement ptrset Andrew Morton
2014-11-13 22:27   ` Tejun Heo
2014-11-13 22:40     ` Andrew Morton
2014-11-14 13:12       ` Tejun Heo
2014-11-18 20:46         ` Andrew Morton
2014-11-18  9:19 ` Lai Jiangshan
2014-11-18 11:55   ` Tejun Heo
2014-11-19  1:41     ` Lai Jiangshan
2014-11-18 15:56 ` Azat Khuzhin
2014-11-18 17:16   ` Tejun Heo
2014-11-18 17:49 ` [PATCH vfs v2 " Tejun Heo
2014-11-25 16:37 ` [PATCH vfs " Jan Kara
2014-12-02 18:15   ` Tejun Heo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=546DF745.1070901@plexistor.com \
    --to=boaz@plexistor.com \
    --cc=akpm@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=hch@infradead.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tj@kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.