Re: [Gluster-devel] readdir() harmful in threaded code
On Mon, Jul 25, 2016 at 05:43:30AM +0200, Emmanuel Dreyfus wrote:
> Vijay Bellur wrote:
>
> > Do you have any concrete examples of problems encountered due to the
> > same directory stream being invoked from multiple threads?
>
> I am not sure this scenario can happen, but what we had were directory
> offsets reused among different DIR * opened on the same directory. This
> works on Linux but is a standard violation, as directory offsets are
> supposed to be valid only for a given DIR *.
>
> It broke NetBSD regression enough that I added a test against it in
> xlator/storage/posix/src:posix.c
>
> seekdir (dir, off);
> #ifndef GF_LINUX_HOST_OS
> if ((u_long)telldir(dir) != off && off != pfd->dir_eof)
> {
> gf_msg (THIS->name, GF_LOG_ERROR, EINVAL,
> P_MSG_DIR_OPERATION_FAILED,
> "seekdir(0x%llx) failed on dir=%p: "
> "Invalid argument (offset reused from "
> "another DIR * structure?)", off, dir);
> errno = EINVAL;
> count = -1;
> goto out;
> }
> #endif /* GF_LINUX_HOST_OS */
>
>
>
> About standards and portability, here is the relevant part in Linux man
> page:
> > In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is not
> > required to be thread-safe. However, in modern implementations
> > (including the glibc implementation), concurrent calls to readdir(3)
> > that specify different directory streams are thread-safe.
> >
> > It is expected that a future version of POSIX.1 will make readdir_r()
> > obsolete, and require that readdir() be thread-safe when concurrently
> > employed on different directory streams.
>
> This means linux recommands using readir(), but such practice is likely
> to break on other systems, since standards do not currently requires it
> to be thread-safe. We can go the readdir() way, but pleas add locks.
>
> Alternatively we can use #ifdef to use alternate code on Linux (readdir)
> and others (readdir_r)
This 2nd approach is indeed what Kaleb implemented in
http://review.gluster.org/14838 and has mentioned in an other reply in
the thread.
HTH,
Niels
signature.asc
Description: PGP signature
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] readdir() harmful in threaded code
On 07/25/2016 07:26 AM, Kaleb S. KEITHLEY wrote: > On 07/23/2016 10:32 AM, Emmanuel Dreyfus wrote: >> Pranith Kumar Karampuri wrote: >> >>> So should we do readdir() with external locks for everything instead? >> >> readdir() with a per-directory lock is safe. However, it may come with a >> performance hit in some scenarios, since two threads cannot read the >> same directory at once. But I am not sure it can happen in GlusterFS. >> >> I am a bit disturbed by readdir_r() being planned for deprecation. The >> Open Group does not say that, or I missed it: >> http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html > > You should take that concern up, perhaps, with the glibc people. > > As for GlusterFS, the recent change I made only affects Linux/glibc. > > Non-linux platforms are unchanged; they use the same old hodgepodge of > readdir(3)/readdir_r(3) they always have. I take that back. Non-linux platforms now use readdir_r(3) exclusively. Which seems to me to be better than the old hodgepodge of readdir(3) and readdir_r(3). > > As such I don't understand what it is that you're concerned about. > -- Kaleb ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] readdir() harmful in threaded code
On 07/23/2016 10:32 AM, Emmanuel Dreyfus wrote: > Pranith Kumar Karampuri wrote: > >> So should we do readdir() with external locks for everything instead? > > readdir() with a per-directory lock is safe. However, it may come with a > performance hit in some scenarios, since two threads cannot read the > same directory at once. But I am not sure it can happen in GlusterFS. > > I am a bit disturbed by readdir_r() being planned for deprecation. The > Open Group does not say that, or I missed it: > http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html You should take that concern up, perhaps, with the glibc people. As for GlusterFS, the recent change I made only affects Linux/glibc. Non-linux platforms are unchanged; they use the same old hodgepodge of readdir(3)/readdir_r(3) they always have. As such I don't understand what it is that you're concerned about. -- Kaleb ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] readdir() harmful in threaded code
Vijay Bellur wrote:
> Do you have any concrete examples of problems encountered due to the
> same directory stream being invoked from multiple threads?
I am not sure this scenario can happen, but what we had were directory
offsets reused among different DIR * opened on the same directory. This
works on Linux but is a standard violation, as directory offsets are
supposed to be valid only for a given DIR *.
It broke NetBSD regression enough that I added a test against it in
xlator/storage/posix/src:posix.c
seekdir (dir, off);
#ifndef GF_LINUX_HOST_OS
if ((u_long)telldir(dir) != off && off != pfd->dir_eof)
{
gf_msg (THIS->name, GF_LOG_ERROR, EINVAL,
P_MSG_DIR_OPERATION_FAILED,
"seekdir(0x%llx) failed on dir=%p: "
"Invalid argument (offset reused from "
"another DIR * structure?)", off, dir);
errno = EINVAL;
count = -1;
goto out;
}
#endif /* GF_LINUX_HOST_OS */
About standards and portability, here is the relevant part in Linux man
page:
> In the current POSIX.1 specification (POSIX.1-2008), readdir(3) is not
> required to be thread-safe. However, in modern implementations
> (including the glibc implementation), concurrent calls to readdir(3)
> that specify different directory streams are thread-safe.
>
> It is expected that a future version of POSIX.1 will make readdir_r()
> obsolete, and require that readdir() be thread-safe when concurrently
> employed on different directory streams.
This means linux recommands using readir(), but such practice is likely
to break on other systems, since standards do not currently requires it
to be thread-safe. We can go the readdir() way, but pleas add locks.
Alternatively we can use #ifdef to use alternate code on Linux (readdir)
and others (readdir_r)
--
Emmanuel Dreyfus
http://hcpnet.free.fr/pubz
m...@netbsd.org
___
Gluster-devel mailing list
Gluster-devel@gluster.org
http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] readdir() harmful in threaded code
On 07/23/2016 03:25 AM, Pranith Kumar Karampuri wrote: Emmanuel, I procrastinated too long on this :-/, It is July already :-(. I just looked at the man page in Linux and it is a bit confusing, so I am not sure how to go ahead. For readdir_r(), I see: DESCRIPTION This function is deprecated; use readdir(3) instead. The readdir_r() function was invented as a reentrant version of readdir(3). It reads the next directory entry from the directory stream dirp, and returns it in the call‐ er-allocated buffer pointed to by entry. For details of the dirent structure, see readir(3). For readdir(3) I see: ATTRIBUTES For an explanation of the terms used in this section, see attributes(7). ┌──┬───┬──┐ │Interface │ Attribute │ Value│ ├──┼───┼──┤ │readdir() │ Thread safety │ MT-Unsafe race:dirstream │ └──┴───┴──┘ In the current POSIX.1 specification (POSIX.1-2008), readdir() is not required to be thread-safe. However, in modern implementations (including the glibc implementation), concur‐ rent calls to readdir() that specify different directory streams are thread-safe. In cases where multiple threads must read from the same directory stream, using readdir() with external synchronization is still preferable to the use of the deprecated readdir_r(3) function. It is expected that a future version of POSIX.1 will require that readdir() be thread-safe when concurrently employed on different directory streams. So should we do readdir() with external locks for everything instead? Do you have any concrete examples of problems encountered due to the same directory stream being invoked from multiple threads? Kaleb's recent patch [1] and the updated man page for readdir_r in linux [2] has some more context. -Vijay [1] http://review.gluster.org/14838 [2] https://manned.org/readdir_r ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] readdir() harmful in threaded code
On Sat, Jul 23, 2016 at 8:02 PM, Emmanuel Dreyfus wrote: > Pranith Kumar Karampuri wrote: > > > So should we do readdir() with external locks for everything instead? > > readdir() with a per-directory lock is safe. However, it may come with a > performance hit in some scenarios, since two threads cannot read the > same directory at once. But I am not sure it can happen in GlusterFS. > > I am a bit disturbed by readdir_r() being planned for deprecation. The > Open Group does not say that, or I missed it: > http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html I will wait for more people to comment on this. Let us see what they think as well. > > > -- > Emmanuel Dreyfus > http://hcpnet.free.fr/pubz > m...@netbsd.org > -- Pranith ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] readdir() harmful in threaded code
Pranith Kumar Karampuri wrote: > So should we do readdir() with external locks for everything instead? readdir() with a per-directory lock is safe. However, it may come with a performance hit in some scenarios, since two threads cannot read the same directory at once. But I am not sure it can happen in GlusterFS. I am a bit disturbed by readdir_r() being planned for deprecation. The Open Group does not say that, or I missed it: http://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html -- Emmanuel Dreyfus http://hcpnet.free.fr/pubz m...@netbsd.org ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] readdir() harmful in threaded code
Emmanuel, I procrastinated too long on this :-/, It is July already :-(. I just looked at the man page in Linux and it is a bit confusing, so I am not sure how to go ahead. For readdir_r(), I see: DESCRIPTION This function is deprecated; use readdir(3) instead. The readdir_r() function was invented as a reentrant version of readdir(3). It reads the next directory entry from the directory stream dirp, and returns it in the call‐ er-allocated buffer pointed to by entry. For details of the dirent structure, see readir(3). For readdir(3) I see: ATTRIBUTES For an explanation of the terms used in this section, see attributes(7). ┌──┬───┬──┐ │Interface │ Attribute │ Value│ ├──┼───┼──┤ │readdir() │ Thread safety │ MT-Unsafe race:dirstream │ └──┴───┴──┘ In the current POSIX.1 specification (POSIX.1-2008), readdir() is not required to be thread-safe. However, in modern implementations (including the glibc implementation), concur‐ rent calls to readdir() that specify different directory streams are thread-safe. In cases where multiple threads must read from the same directory stream, using readdir() with external synchronization is still preferable to the use of the deprecated readdir_r(3) function. It is expected that a future version of POSIX.1 will require that readdir() be thread-safe when concurrently employed on different directory streams. So should we do readdir() with external locks for everything instead? On Thu, Feb 11, 2016 at 2:35 PM, Emmanuel Dreyfus wrote: > Juste to make sure there is no misunderstanding here: unfortunately I > do not have time right now to submit a fix. It would be nice if someone > else coule look at it. > > On Wed, Feb 10, 2016 at 01:48:52PM +, Emmanuel Dreyfus wrote: > > Hi > > > > After obtaining a core in a regression, I noticed there are a few > readdir() > > use in threaded code. This is begging for a crash, as readdir() maintains > > an internal state that will be trashed on concurent use. readdir_r() > > should be used instead. > > > > A quick search shows readdir(à usage here: > > contrib/fuse-util/mount_util.c:30 > > extras/test/ld-preload-test/ld-preload-test.c:310 > > extras/test/test-ffop.c:550 > > libglusterfs/src/compat.c:256 > > libglusterfs/src/compat.c:315 > > libglusterfs/src/syscall.c:97 > > tests/basic/fops-sanity.c:662 > > tests/utils/arequal-checksum.c:331 > > > > Occurences in contrib, extra and tests are probably harmless are there > > are usage in standalone programs that are not threaded. We are left with > > three groups of problems: > > > > 1) libglusterfs/src/compat.c:256 and libglusterfs/src/compat.c:315 > > This is Solaris compatibility code. Is it used at all? > > > > 2) libglusterfs/src/syscall.c:97 This is the sys_readdir() wrapper, > > which is in turn used in: > > libglusterfs/src/run.c:284 > > xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c:582 > > xlators/features/changelog/lib/src/gf-history-changelog.c:854 > > xlators/features/index/src/index.c:471 > > xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c > > xlators/storage/posix/src/posix.c:3700 > > xlators/storage/posix/src/posix.c:5896 > > > > 3) We also find sys_readdir() in libglusterfs/src/common-utils.h for > > GF_FOR_EACH_ENTRY_IN_DIR() which in turn appears in: > > libglusterfs/src/common-utils.c:3979 > > libglusterfs/src/common-utils.c:4002 > > xlators/mgmt/glusterd/src/glusterd-hooks.c:365 > > xlators/mgmt/glusterd/src/glusterd-hooks.c:379 > > xlators/mgmt/glusterd/src/glusterd-store.c:651 > > xlators/mgmt/glusterd/src/glusterd-store.c:661 > > xlators/mgmt/glusterd/src/glusterd-store.c:1781 > > xlators/mgmt/glusterd/src/glusterd-store.c:1806 > > xlators/mgmt/glusterd/src/glusterd-store.c:3044 > > xlators/mgmt/glusterd/src/glusterd-store.c:3072 > > xlators/mgmt/glusterd/src/glusterd-store.c:3593 > > xlators/mgmt/glusterd/src/glusterd-store.c:3606 > > xlators/mgmt/glusterd/src/glusterd-store.c:4032 > > xlators/mgmt/glusterd/src/glusterd-store.c:4111 > > > > There a hive of sprious bugs to squash here. > > > > -- > > Emmanuel Dreyfus > > m...@netbsd.org > > ___ > > Gluster-devel mailing list > > Gluster-devel@gluster.org > > http://www.gluster.org/mailman/listinfo/gluster-devel > > -- > Emmanuel Dreyfus > m...@netbsd.org > ___ > Gluster-devel mailing list > Gluster-devel@gluster.org > http://www.gluster.org/mailman/listinfo/gluster-devel > -- Pranith ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
Re: [Gluster-devel] readdir() harmful in threaded code
Juste to make sure there is no misunderstanding here: unfortunately I do not have time right now to submit a fix. It would be nice if someone else coule look at it. On Wed, Feb 10, 2016 at 01:48:52PM +, Emmanuel Dreyfus wrote: > Hi > > After obtaining a core in a regression, I noticed there are a few readdir() > use in threaded code. This is begging for a crash, as readdir() maintains > an internal state that will be trashed on concurent use. readdir_r() > should be used instead. > > A quick search shows readdir(à usage here: > contrib/fuse-util/mount_util.c:30 > extras/test/ld-preload-test/ld-preload-test.c:310 > extras/test/test-ffop.c:550 > libglusterfs/src/compat.c:256 > libglusterfs/src/compat.c:315 > libglusterfs/src/syscall.c:97 > tests/basic/fops-sanity.c:662 > tests/utils/arequal-checksum.c:331 > > Occurences in contrib, extra and tests are probably harmless are there > are usage in standalone programs that are not threaded. We are left with > three groups of problems: > > 1) libglusterfs/src/compat.c:256 and libglusterfs/src/compat.c:315 > This is Solaris compatibility code. Is it used at all? > > 2) libglusterfs/src/syscall.c:97 This is the sys_readdir() wrapper, > which is in turn used in: > libglusterfs/src/run.c:284 > xlators/features/bit-rot/src/stub/bit-rot-stub-helpers.c:582 > xlators/features/changelog/lib/src/gf-history-changelog.c:854 > xlators/features/index/src/index.c:471 > xlators/mgmt/glusterd/src/glusterd-snapshot-utils.c > xlators/storage/posix/src/posix.c:3700 > xlators/storage/posix/src/posix.c:5896 > > 3) We also find sys_readdir() in libglusterfs/src/common-utils.h for > GF_FOR_EACH_ENTRY_IN_DIR() which in turn appears in: > libglusterfs/src/common-utils.c:3979 > libglusterfs/src/common-utils.c:4002 > xlators/mgmt/glusterd/src/glusterd-hooks.c:365 > xlators/mgmt/glusterd/src/glusterd-hooks.c:379 > xlators/mgmt/glusterd/src/glusterd-store.c:651 > xlators/mgmt/glusterd/src/glusterd-store.c:661 > xlators/mgmt/glusterd/src/glusterd-store.c:1781 > xlators/mgmt/glusterd/src/glusterd-store.c:1806 > xlators/mgmt/glusterd/src/glusterd-store.c:3044 > xlators/mgmt/glusterd/src/glusterd-store.c:3072 > xlators/mgmt/glusterd/src/glusterd-store.c:3593 > xlators/mgmt/glusterd/src/glusterd-store.c:3606 > xlators/mgmt/glusterd/src/glusterd-store.c:4032 > xlators/mgmt/glusterd/src/glusterd-store.c:4111 > > There a hive of sprious bugs to squash here. > > -- > Emmanuel Dreyfus > m...@netbsd.org > ___ > Gluster-devel mailing list > Gluster-devel@gluster.org > http://www.gluster.org/mailman/listinfo/gluster-devel -- Emmanuel Dreyfus m...@netbsd.org ___ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel
