Bug 2922 - FreeBSD AIO patches
FreeBSD AIO patches
Status: ASSIGNED
Product: Samba 3.0
Classification: Unclassified
Component: File Services
3.0.20
All FreeBSD
: P3 normal
: none
Assigned To: Jeremy Allison
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-07-26 20:07 UTC by Timur Bakeyev
Modified: 2007-06-28 15:41 UTC (History)
1 user (show)

See Also:


Attachments
Configure checks (2.57 KB, patch)
2005-07-26 20:07 UTC, Timur Bakeyev
no flags Details
AIO code (2.75 KB, patch)
2005-07-26 20:08 UTC, Timur Bakeyev
no flags Details
sival_int based code (2.29 KB, patch)
2005-07-27 17:23 UTC, Timur Bakeyev
no flags Details
patch (1.77 KB, patch)
2005-10-24 22:28 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Timur Bakeyev 2005-07-26 20:07:22 UTC
Hi Jeremy!

Here I attach patches, that make AIO code COMPILABLE under FreeBSD. Due
incomplete implementation of signal handling on sigaction() for AIO code it
doesn't work at least for FreeBSD 5.x. Still, for completeness, I think, this
code should be there.

Basically, changes are quite simple.

In configure:

Amasingly, members of sigevent struct in BSD have a bit different name,
sigval_int vs. sival_int. Weird, I know. So, there is a check against structure
members and later usage in the code of defined macros.

BTW, there is usage of aio.h to find timespec struct - I don't think it's
necessary. But I could be wrong :)

Second check is for the presence of aio_read. In BSD this function(s) is a part
of the libc, so no need to link against(non-existing) librt. I hope, this is a
right code, but feel free to modify it.

As for the aio.c patch:

There is no SIGRTMIN macro in BSD, so NSIG is used as the boundary. Maybe,
adding 3 to it would be nice too. Other macro used to mask difference in the
names of structure members.

Another change is quiter important - usage of sival_int instead of sival_ptr.
There are two reasons - one, is that int is more than enough to pass mid. Second
is is more serious - as on BSD this elements are not initialised, value is
always a 0, which just sigsev when you try to dereference it. So, we should
either use int instead and avoid conversion or at least, check, that value not
zero. Of cource, that should be consistent cross the code.

And last change - in handle_aio_completed - a chech, that aio_ex is not NULL.
It's the result of failed find_aio_ex and not guarded at least in
wait_for_aio_completion.

With that minor changes aio code compiles on FreeBSD. It still gives short read,
as the mid not really passed on signal delivery, but I hope it'll be fixed at
least in 6.x.

Sorry for being so verbose :) Just wanted to make things clear :)

With regards,
Timur.
Comment 1 Timur Bakeyev 2005-07-26 20:07:56 UTC
Created attachment 1338 [details]
Configure checks
Comment 2 Timur Bakeyev 2005-07-26 20:08:17 UTC
Created attachment 1339 [details]
AIO code
Comment 3 Jeremy Allison 2005-07-26 20:45:20 UTC
This part of your patch :

 	if (signals_received < AIO_PENDING_SIZE - 1) {
-		aio_pending_array[signals_received] = *(uint16 *)(info->si_value.sival_ptr);
+		aio_pending_array[signals_received] = (uint16)(info->si_value.sival_int);
 		signals_received++;

makes no sense and will break the Linux implementation. Note that the original
code sets sival_ptr to be :

       a->aio_sigevent.sigev_value.sival_ptr = (void *)&aio_ex->mid;

That's the *address* of aio_ex->mid, not aio_ex->mid itself. Your patch casts
the value into a uint16. It isn't - it's a *pointer*. Please tidy this up and
remove some of the spurious whitespace changes. I need to see a much small patch
before I can accept this, sorry.
Jeremy.
Comment 4 Timur Bakeyev 2005-07-26 21:12:02 UTC
(In reply to comment #3)
>
> makes no sense and will break the Linux implementation. Note that the original
> code sets sival_ptr to be :
> 
>        a->aio_sigevent.sigev_value.sival_ptr = (void *)&aio_ex->mid;
> 
> That's the *address* of aio_ex->mid, not aio_ex->mid itself. Your patch casts
> the value into a uint16. It isn't - it's a *pointer*. Please tidy this up and
> remove some of the spurious whitespace changes. I need to see a much small patch
> before I can accept this, sorry.

As I wrote in my longish comment, I need your decission - would you preffer to
just switch to passing int(as mid fits there) or to put a guardian code around
that assignment, that will check, that pointer isn't a zero.

Both approaches are ok and it's just a question of your preference.

Cheers,
Timur.

Comment 5 Timur Bakeyev 2005-07-27 17:23:38 UTC
Created attachment 1340 [details]
sival_int based code

This is the variant, that uses sival_int to pass the mid.
Comment 6 Gerald (Jerry) Carter 2005-08-12 11:34:37 UTC
Jeremy,  could you see if the current patches still need more work?
If so, then we can look for inclusion in 3.0.21
Comment 7 Jeremy Allison 2005-10-24 15:14:04 UTC
Ok I'm happy with the change from sival_ptr to sival_int. I'll try and get this
in for 3.0.21.
Jeremy.
Comment 8 Jeremy Allison 2005-10-24 22:28:05 UTC
Created attachment 1536 [details]
patch

Is this enough for FreeBSD ? Or do you need more ?
Jeremy.
Comment 9 Timur Bakeyev 2005-10-25 05:13:51 UTC
(In reply to comment #8)
> Created an attachment (id=1536) [edit]
> patch
> 
> Is this enough for FreeBSD ? Or do you need more ?
> Jeremy.

At least, it's also necessary to run configure
checks(https://bugzilla.samba.org/attachment.cgi?id=1338) and use it's results:

--- aio.c.orig	Thu Jul 28 01:14:47 2005
+++ aio.c	Thu Jul 28 01:18:43 2005
@@ -25,7 +25,17 @@
 
 /* The signal we'll use to signify aio done. */
 #ifndef RT_SIGNAL_AIO
-#define RT_SIGNAL_AIO (SIGRTMIN+3)
+#ifndef SIGRTMIN
+#define SIGRTMIN	NSIG
+#endif
+#define RT_SIGNAL_AIO	(SIGRTMIN+3)
+#endif
+
+#ifndef HAVE_STRUCT_SIGEVENT_SIGEV_VALUE_SIVAL_PTR
+#ifdef HAVE_STRUCT_SIGEVENT_SIGEV_VALUE_SIGVAL_PTR
+#define sival_int	sigval_int
+#define sival_ptr	sigval_ptr
+#endif
 #endif

> There is no SIGRTMIN macro in BSD, so NSIG is used as the boundary. Maybe,
> adding 3 to it would be nice too. Other macro used to mask difference in the
> names of structure members.
>
> And last change - in handle_aio_completed - a chech, that aio_ex is not NULL.
> It's the result of failed find_aio_ex and not guarded at least in
> wait_for_aio_completion.

It's also maybe reasonable to leave that sanity check:

@@ -499,6 +509,11 @@
 static BOOL handle_aio_completed(struct aio_extra *aio_ex, int *perr)
 {
 	int err;
+	
+	if(!aio_ex) {
+	        DEBUG(3, ("handle_aio_completed: Non-existing aio_ex passed\n"));
+		return False;
+	}

The rest, I guess, is fine.

With regards,
Timur
Comment 10 Jeremy Allison 2005-10-25 11:31:11 UTC
Ok I may put the SIGRTMIN definition in the configure tests - SIGRTMIN is a
POSIX realtime requirement, so it *should* be there (this is a bug in FreeBSD
IMHO). The other check isn't right - we *need* to crash in this case (IMHO).
I've guarded it in all places it's called - we don't want this to silently fail.
Jeremy.
Comment 11 Timur Bakeyev 2005-10-26 04:14:29 UTC
(In reply to comment #10)
> Ok I may put the SIGRTMIN definition in the configure tests - SIGRTMIN is a
> POSIX realtime requirement, so it *should* be there (this is a bug in FreeBSD
> IMHO). The other check isn't right - we *need* to crash in this case (IMHO).
> I've guarded it in all places it's called - we don't want this to silently fail.
> Jeremy.

I guess, configure check is a right thing(tm). And agree, it is something wrong
on FreeBSD side...

As for the sanity check - I think that sigsegv isn't the best way of shuting
down the server. So, at least an assertion would be nice... Still I'm not sure,
that AIO logic ensures, that all the signals will be delivered/handled. But I'm
not very familiar with it anyhow :)

Cheers,
Timur.