FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor


 
 
LinkBack Thread Tools
 
Old 10-17-2011, 04:22 PM
Mikulas Patocka
 
Default dm-bufio

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Mon, Oct 17, 2011 at 10:05:26AM -0400, Mikulas Patocka wrote:
> >
> >
> > On Mon, 17 Oct 2011, Joe Thornber wrote:
> >
> > > On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > > @@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
> > > static void submit_io(struct dm_buffer *b, int rw, sector_t block,
> > > bio_end_io_t *end_io)
> > > {
> > > - if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> > > - b->data_mode != DATA_MODE_VMALLOC)
> > > + if (rw == WRITE && b->c->write_callback)
> > > + b->c->write_callback(b);
> > > if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
> > > likely(b->data_mode != DATA_MODE_VMALLOC))
> > > use_inline_bio(b, rw, block, end_io);
> > > else
> > > use_dmio(b, rw, block, end_io);
> > > @@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct
> > > clear_bit(B_DIRTY, &b->state);
> > > wait_on_bit_lock(&b->state, B_WRITING,
> > > do_io_schedule, TASK_UNINTERRUPTIBLE);
> > > - if (b->c->write_callback)
> > > - b->c->write_callback(b);
> > > submit_io(b, WRITE, b->block, write_endio);
> > > }
> > >
> > >
> > > This doesn't seem an improvement. Except ... it changes the behaviour
> > > of dm_bufio_release_move(). So was there a preexisting bug in
> > > dm_bufio_release_move() that you're trying to fix with this patch?
> >
> > The actual reason was to do this callback in dm_bufio_release_move() too
> > --- just for consistency. (the user of dm_bufio_release_move() doesn't use
> > write_callback anyway).
>
> thinp uses dm_bufio_release_move() and write_callback. So yes, this
> is a bug fix. I thought so and merged.

BTW. it still uses the old block number in "prepare_for_write" callback.
Do you use this block number somewhere? Should we link the buffer to the
new block before the call and the link it back?

Mikulas

> - Joe
>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-17-2011, 04:24 PM
Mikulas Patocka
 
Default dm-bufio

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > Hi
> >
> > This is a patch for dm-bufio.
>
> I've merged all I'm going to at this point and pushed to thin-dev.

I think Alasdair currently holds the master version of this code. Or is it
you? If we had three versions of the same code and don't know which one is
the master, then there is a problem.

Mikulas

> If you put together a patch for the cond_resched stuff I'll take that;
> providing you don't call it in a tight loop like here:
>
> > @@ -758,8 +772,9 @@ static struct dm_buffer *__find(struct d
> > struct dm_buffer *b;
> > struct hlist_node *hn;
> > hlist_for_each_entry(b, hn, &c->cache_hash[DM_BUFIO_HASH(block)], hash_list) {
> > - if (b->block == block)
> > + if (likely(b->block == block))
> > return b;
> > + dm_bufio_cond_resched();
> > }
>
>
> I'm not really interested in the likely/unlikely annotations.
>
> - Joe
>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-17-2011, 04:41 PM
Joe Thornber
 
Default dm-bufio

On Mon, Oct 17, 2011 at 12:22:13PM -0400, Mikulas Patocka wrote:
> > thinp uses dm_bufio_release_move() and write_callback. So yes, this
> > is a bug fix. I thought so and merged.
>
> BTW. it still uses the old block number in "prepare_for_write" callback.
> Do you use this block number somewhere? Should we link the buffer to the
> new block before the call and the link it back?

Good catch, let me look into it.

- Joe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-17-2011, 04:43 PM
Joe Thornber
 
Default dm-bufio

On Mon, Oct 17, 2011 at 12:24:00PM -0400, Mikulas Patocka wrote:
> On Mon, 17 Oct 2011, Joe Thornber wrote:
>
> > On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > > Hi
> > >
> > > This is a patch for dm-bufio.
> >
> > I've merged all I'm going to at this point and pushed to thin-dev.
>
> I think Alasdair currently holds the master version of this code. Or is it
> you? If we had three versions of the same code and don't know which one is
> the master, then there is a problem.

I consider my tree to be the master of thin-provisioning, and will be
very annoyed if anything gets pushed to linux-next that hasn't been
sent to me first. dm-bufio is yours to maintain in the long term -
I'm just taking an interest because it's blocking acceptance of my
code.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-17-2011, 04:49 PM
Mikulas Patocka
 
Default dm-bufio

On Fri, 14 Oct 2011, Joe Thornber wrote:

> On Thu, Oct 13, 2011 at 11:05:51AM -0400, Mikulas Patocka wrote:
> > Hi
> >
> > I found a way how to shut up lockdep warnings in dm-bufio, so you can
> > download new a version which uses nested locks from
> > http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/
>
> Thanks Mikulas, I'd had a couple of people ask me about these warnings.
>
> > BTW. why did you move dm-bufio to persistent-data directory? What are
> > other dm-bufio users (such as dm-zeroed or dm-multisnap) going to do? I
> > thought dm-bufio should be a separate module available to all device
> > mapper targets, such as dm-io, dm-kcopyd or so?
>
> agk made it clear he wasn't going to merge bufio when I met with him a
> couple of weeks ago. So to try and speed things up I put my 'agk' hat
> on and tidied as I thought he would. The main things I did were
> remove the gratuitous gotos (eg, jumping from one branch of an if to
> another).

My point is that - to get understandable code, each function should do a
task that can be easily and simply described. It is not necessary that the
function body is small or that it dosn't contain gotos, what is required
is that the description of a function is small.

If you replace random parts of a function body with a call to another
function in order to eliminate a goto, it doesn't improve readability of
the code at all. Instead of a goto you get a call - both are control
transfer constructs and a call is not necessarily easier to follow than a
goto (unless the condition in the first paragraph is true) - as it can be
seen that both you and me made some bugs on those new
goto-replaced-with-call functions.

If you want to do it, just do it, but it makes no sense to me.

Mikulas

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-17-2011, 06:01 PM
Joe Thornber
 
Default dm-bufio

On Mon, Oct 17, 2011 at 12:49:22PM -0400, Mikulas Patocka wrote:
> If you want to do it, just do it, but it makes no sense to me.

Well Alasdair, Mike and myself have all looked at this code at various
times over the last 18 months and come to similar conclusions. We
find the following logic difficult with all those gotos. There is
also some redundancy which I only noticed when trying to refactor it
(repeated checks). There's an example that has a jump from one branch
of an 'if' to another, it's trivial to factor out into a function for
each branch, yet you insist on arguing and refuse to change it. I'm
perfectly prepared to accept that you're familiar enough with this
code, or just clever enough for it to be transparent to you. But it's
the sort of thing that's going to get picked up on by other small
brained developers like me. Choose your battles wisely, and save your
energies for something more interesting.

- Joe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 10-17-2011, 06:30 PM
Joe Thornber
 
Default dm-bufio

On Mon, Oct 17, 2011 at 12:22:13PM -0400, Mikulas Patocka wrote:
> > thinp uses dm_bufio_release_move() and write_callback. So yes, this
> > is a bug fix. I thought so and merged.
>
> BTW. it still uses the old block number in "prepare_for_write" callback.
> Do you use this block number somewhere? Should we link the buffer to the
> new block before the call and the link it back?

I double checked this; it's fine. I use release_move as part of the
shadow operation in the transaction manager. As such we know a write
lock is going to be grabbed on it straight away (which will dirty and
prompt a rewriting of the block nr). Also in the event of a crash
before the next commit, the new block wont appear in the metadata at
all.

- Joe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-23-2012, 10:01 AM
"Kasatkin, Dmitry"
 
Default dm-bufio

Hello,

When using dm-bufio and dm-io in general, how to ensure that all dirty
buffers are written to the storage when machine reboots?
suspend hooks could be used, but they are not called on reboot, only
when suspending/removing the target...


Thanks,
Dmitry

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-23-2012, 10:10 AM
Zdenek Kabelac
 
Default dm-bufio

Dne 23.3.2012 12:01, Kasatkin, Dmitry napsal(a):
> Hello,
>
> When using dm-bufio and dm-io in general, how to ensure that all dirty
> buffers are written to the storage when machine reboots?
> suspend hooks could be used, but they are not called on reboot, only
> when suspending/removing the target...
>

You mean you reboot without running 'sync' command?

And yes - on reboot you should properly unmount devices - so you should
see removal of target on your shutdown sequence - I believe Fedora currently
tries to support switch to some shutdown ramdisk, so all filesystem and
devices might be properly unmounted and destroyed.

Zdenek


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 03-23-2012, 10:26 AM
"Kasatkin, Dmitry"
 
Default dm-bufio

On Fri, Mar 23, 2012 at 1:10 PM, Zdenek Kabelac <zkabelac@redhat.com> wrote:
> Dne 23.3.2012 12:01, Kasatkin, Dmitry napsal(a):
>> Hello,
>>
>> When using dm-bufio and dm-io in general, how to ensure that all dirty
>> buffers are written to the storage when machine reboots?
>> suspend hooks could be used, but they are not called on reboot, only
>> when suspending/removing the target...
>>
>
> You mean you reboot without running *'sync' command?
>
> And yes - on reboot you should properly unmount devices - so you should
> see removal of target on your shutdown sequence - *I believe Fedora currently
> tries to support switch to some shutdown ramdisk, so all filesystem and
> devices might be properly unmounted and destroyed.
>

Hello,

Thanks for response.
I use bufio to store some data on block device.
It is not mounted in anyway. My target just use it to load/store data.
When machine reboots, I want to be sure that bufio written all dirty buffers...

- Dmitry


> Zdenek
>
>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 02:09 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org