Date:	Mon, 1 Oct 2012 17:22:16 -0700
From:	Kent Overstreet <koverstreet@...gle.com>
To:	Zach Brown <zab@...bo.net>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	tytso@...gle.com, tj@...nel.org,
	Dave Kleikamp <dave.kleikamp@...cle.com>,
	Dmitry Monakhov <dmonakhov@...nvz.org>,
	"Maxim V. Patlasov" <mpatlasov@...allels.com>,
	michael.mesnier@...el.com, jeffrey.d.skirvin@...el.com,
	Martin Petersen <martin.petersen@...cle.com>
Subject: Re: [RFC, PATCH] Extensible AIO interface

On Mon, Oct 01, 2012 at 04:44:39PM -0700, Zach Brown wrote:
> > Not just per sector, Per hardware sector. For passing around checksums
> > userspace would have to find out the hardware sector size and checksum
> > type/size via a different interface, and then the attribute would
> > contain a pointer to a buffer that can hold the appropriate number of
> > checksums.
> 
> All problems fall to another layer of indirection? :) 

Yep :)

> But yes, that's
> fair.  I was obviously just assuming that the checksums would be in the
> attribute.
> 
> But now we're talking about layers of user pointers.  Would the
> attribute parser need to verify/copy pointers before downstream kernel
> code tries to work with it?  Would it be up to the attribute consumers
> to verify the pointers that the core doesn't really touch? 

The generic code wouldn't know about any user pointers inside
attributes, so it'd have to be downstream consumers. Hopefully there
won't be many attributes with user pointers in them (I don't expect
there to be), so we won't have too much of this messyness.

> Are these
> second pointers native (enter compat goo) or u64s?

Outside the scope of the generic implementation, but AFAIK u64s are the
sane way so I'd prefer to just enforce that.

> > I don't think there's anything fragile about the basic idea though. Or
> > do you have some way of improving upon it in mind?
> 
> Nothing super great is springing to mind, no.
> 
> > The idea with the size field is that it's just sizeof(the particular
> > attribute struct), so when userspace is appending attributes it just
> > sets size = sizeof() and attr_list->size += attr->size.
> 
> I suppose.  But this also raises the spectre of aligning the packed
> attributes to match their struct definitions.  It's the netlink(3)
> macros all over again, right?  I guess unaligned accesses aren't *that*
> big a deal.  But still.

I was just thinking about that a few minutes ago. Since the way I'm
doing it embeds struct iocb_attr inside the specific attributes, if we
stick __aligned(8) on struct iocb_attr that should solve alignment.

> And what about duplicate instances of a given attribute id?  Use the
> first?  The last?  Error?  Depends on the id?

I suspect duplicate instances will be useful and the sanest approach
for a few things, so I don't want to disallow it.

But - perhaps we could define whether duplicates are allowed of a given
attribute along with the attribute ids, then the kernel in generic code
could check for duplicates of attrs for which it was disallowed and
error.

I think I really like that idea.

> It just seems like there are a lot of corner cases that can go wrong
> with an API that is so free form.  I'd like something a lot harder to
> make mistakes with.
> 
> - z
> (being That Guy today, apparently :/)

I think it's got to be free form to be useful, but that doesn't mean we
can't avoid as many mistakes as possible ahead of time so please
continue to point out anything you can think of :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists